From 6c4d1e88a43e71758016f59dd90397c2103b6014 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 21 May 2025 13:15:50 -0700 Subject: [PATCH 1/3] fix --- src/wasm/wasm-ir-builder.cpp | 21 ++++++++++++++++++--- test/lit/branch-hinting.wast | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 5052310f530..49ecc6c38c3 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -151,9 +151,20 @@ void IRBuilder::push(Expression* expr) { applyDebugLoc(expr); if (binaryPos && func && lastBinaryPos != *binaryPos) { - func->expressionLocations[expr] = - BinaryLocations::Span{BinaryLocation(lastBinaryPos - codeSectionOffset), - BinaryLocation(*binaryPos - codeSectionOffset)}; + auto start = BinaryLocation(lastBinaryPos - codeSectionOffset); + auto end = BinaryLocation(*binaryPos - codeSectionOffset); + // Some expressions already have their start noted, and we are just seeing + // their last segment (like an Else). + // TODO: does Try etc. need this too? + if (expr->is()) { + auto iter = func->expressionLocations.find(expr); + assert(iter != func->expressionLocations.end()); + iter->second.end = end; + // The true start from before is before the start of the current segment. + assert(iter->second.start < start); + } else { + func->expressionLocations[expr] = BinaryLocations::Span{start, end}; + } lastBinaryPos = *binaryPos; } @@ -933,6 +944,10 @@ Result<> IRBuilder::visitElse() { if (binaryPos && func) { func->delimiterLocations[iff][BinaryLocations::Else] = lastBinaryPos - codeSectionOffset; + + // Note the start of the if (which will be lost as the If is closed and the + // Else begins, but the if spans them both). + func->expressionLocations[iff].start = scope.startPos - codeSectionOffset; } return pushScope(ScopeCtx::makeElse(std::move(scope))); diff --git a/test/lit/branch-hinting.wast b/test/lit/branch-hinting.wast index 07277f0bc67..8ca0442c45d 100644 --- a/test/lit/branch-hinting.wast +++ b/test/lit/branch-hinting.wast @@ -259,6 +259,38 @@ ) ) + (func $branch-hints-if-else (param $x i32) + (@metadata.code.branch_hint "\01") + (if + (local.get $x) + (then + (drop + (i32.const 1) + ) + ) + (else + (drop + (i32.const 0) + ) + ) + ) + ) + + (func $branch-hints-if-else-result (param $x i32) + (drop + (@metadata.code.branch_hint "\01") + (if (result i32) + (local.get $x) + (then + (i32.const 1) + ) + (else + (i32.const 0) + ) + ) + ) + ) + ;; CHECK: (func $branch-hints-br_on (type $1) (param $x anyref) ;; CHECK-NEXT: (block $out ;; CHECK-NEXT: (drop From 11f2c9a33b6c7d9f69c1dcaac555d3a9cdee4c19 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 21 May 2025 13:17:56 -0700 Subject: [PATCH 2/3] finish --- src/wasm/wasm-ir-builder.cpp | 8 ++-- test/lit/branch-hinting.wast | 90 ++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 49ecc6c38c3..c9749f0610f 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -155,14 +155,14 @@ void IRBuilder::push(Expression* expr) { auto end = BinaryLocation(*binaryPos - codeSectionOffset); // Some expressions already have their start noted, and we are just seeing // their last segment (like an Else). - // TODO: does Try etc. need this too? - if (expr->is()) { - auto iter = func->expressionLocations.find(expr); - assert(iter != func->expressionLocations.end()); + auto iter = func->expressionLocations.find(expr); + if (iter != func->expressionLocations.end()) { + // Just update the end. iter->second.end = end; // The true start from before is before the start of the current segment. assert(iter->second.start < start); } else { + // Add a whole entry. func->expressionLocations[expr] = BinaryLocations::Span{start, end}; } lastBinaryPos = *binaryPos; diff --git a/test/lit/branch-hinting.wast b/test/lit/branch-hinting.wast index 8ca0442c45d..ba8f3716c3f 100644 --- a/test/lit/branch-hinting.wast +++ b/test/lit/branch-hinting.wast @@ -259,6 +259,54 @@ ) ) + ;; CHECK: (func $branch-hints-if-else (type $0) (param $x i32) + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; RTRIP: (func $branch-hints-if-else (type $0) (param $x i32) + ;; RTRIP-NEXT: (@metadata.code.branch_hint "\01") + ;; RTRIP-NEXT: (if + ;; RTRIP-NEXT: (local.get $x) + ;; RTRIP-NEXT: (then + ;; RTRIP-NEXT: (drop + ;; RTRIP-NEXT: (i32.const 1) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: (else + ;; RTRIP-NEXT: (drop + ;; RTRIP-NEXT: (i32.const 0) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; SRCMP: (func $branch-hints-if-else (type $0) (param $x i32) + ;; SRCMP-NEXT: (@metadata.code.branch_hint "\01") + ;; SRCMP-NEXT: (if + ;; SRCMP-NEXT: (local.get $x) + ;; SRCMP-NEXT: (then + ;; SRCMP-NEXT: (drop + ;; SRCMP-NEXT: (i32.const 1) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: (else + ;; SRCMP-NEXT: (drop + ;; SRCMP-NEXT: (i32.const 0) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) (func $branch-hints-if-else (param $x i32) (@metadata.code.branch_hint "\01") (if @@ -276,6 +324,48 @@ ) ) + ;; CHECK: (func $branch-hints-if-else-result (type $0) (param $x i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (@metadata.code.branch_hint "\01") + ;; CHECK-NEXT: (if (result i32) + ;; CHECK-NEXT: (local.get $x) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; RTRIP: (func $branch-hints-if-else-result (type $0) (param $x i32) + ;; RTRIP-NEXT: (drop + ;; RTRIP-NEXT: (@metadata.code.branch_hint "\01") + ;; RTRIP-NEXT: (if (result i32) + ;; RTRIP-NEXT: (local.get $x) + ;; RTRIP-NEXT: (then + ;; RTRIP-NEXT: (i32.const 1) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: (else + ;; RTRIP-NEXT: (i32.const 0) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; RTRIP-NEXT: ) + ;; SRCMP: (func $branch-hints-if-else-result (type $0) (param $x i32) + ;; SRCMP-NEXT: (drop + ;; SRCMP-NEXT: (@metadata.code.branch_hint "\01") + ;; SRCMP-NEXT: (if (result i32) + ;; SRCMP-NEXT: (local.get $x) + ;; SRCMP-NEXT: (then + ;; SRCMP-NEXT: (i32.const 1) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: (else + ;; SRCMP-NEXT: (i32.const 0) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) + ;; SRCMP-NEXT: ) (func $branch-hints-if-else-result (param $x i32) (drop (@metadata.code.branch_hint "\01") From 3cbc86c23a93ef75d6dac771e5e00a78362a40f0 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 21 May 2025 14:37:55 -0700 Subject: [PATCH 3/3] feedback --- src/wasm/wasm-ir-builder.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index c9749f0610f..1fb39b14ec6 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -151,19 +151,17 @@ void IRBuilder::push(Expression* expr) { applyDebugLoc(expr); if (binaryPos && func && lastBinaryPos != *binaryPos) { - auto start = BinaryLocation(lastBinaryPos - codeSectionOffset); - auto end = BinaryLocation(*binaryPos - codeSectionOffset); + auto span = + BinaryLocations::Span{BinaryLocation(lastBinaryPos - codeSectionOffset), + BinaryLocation(*binaryPos - codeSectionOffset)}; // Some expressions already have their start noted, and we are just seeing // their last segment (like an Else). - auto iter = func->expressionLocations.find(expr); - if (iter != func->expressionLocations.end()) { + auto [iter, inserted] = func->expressionLocations.insert({expr, span}); + if (!inserted) { // Just update the end. - iter->second.end = end; + iter->second.end = span.end; // The true start from before is before the start of the current segment. - assert(iter->second.start < start); - } else { - // Add a whole entry. - func->expressionLocations[expr] = BinaryLocations::Span{start, end}; + assert(iter->second.start < span.start); } lastBinaryPos = *binaryPos; } @@ -995,6 +993,8 @@ Result<> IRBuilder::visitCatch(Name tag) { if (binaryPos && func) { auto& delimiterLocs = func->delimiterLocations[tryy]; delimiterLocs[delimiterLocs.size()] = lastBinaryPos - codeSectionOffset; + // TODO: As in visitElse, we likely need to stash the Try start. Here we + // also need to account for multiple catches. } CHECK_ERR(pushScope(ScopeCtx::makeCatch(std::move(scope), tryy)));