Skip to content

[WebAssembly] Fold TargetGlobalAddress with added offset #145829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukel97 merged 8 commits into from
Jul 3, 2025
Merged

[WebAssembly] Fold TargetGlobalAddress with added offset #145829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fe369c1
Precommit test for folding a load of unsigned offset
badumbatish Jun 25, 2025
3dfe71a
[WebAssembly] Fold nuw global add to load offset
badumbatish Jun 26, 2025
0a00fda
[WebAssembly] Fix nit from PR 145829
Jul 1, 2025
91b42d5
[WebAssembly] Add guard for global folding
Jul 1, 2025
65f68c3
[WebAssembly] Fix nits for PR 145829
Jul 2, 2025
3e7f049
Apply suggestions from nit
badumbatish Jul 3, 2025
d501edb
Merge branch 'main' of https://github.com/llvm/llvm-project into fold…
Jul 3, 2025
6418cf6
Fix whitespace
lukel97 Jul 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,17 +343,29 @@ bool WebAssemblyDAGToDAGISel::SelectAddrAddOperands(MVT OffsetType, SDValue N,
if (N.getOpcode() == ISD::ADD && !N.getNode()->getFlags().hasNoUnsignedWrap())
return false;

// Folds constants in an add into the offset.
for (size_t i = 0; i < 2; ++i) {
SDValue Op = N.getOperand(i);
SDValue OtherOp = N.getOperand(i == 0 ? 1 : 0);

// Folds constants in an add into the offset.
if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Op)) {
Offset =
CurDAG->getTargetConstant(CN->getZExtValue(), SDLoc(N), OffsetType);
Addr = OtherOp;
return true;
}

// Fold target global addresses into the offset.
if (!TM.isPositionIndependent()) {
if (Op.getOpcode() == WebAssemblyISD::Wrapper)
Op = Op.getOperand(0);

if (Op.getOpcode() == ISD::TargetGlobalAddress) {
Addr = OtherOp;
Offset = Op;
return true;
}
}
}
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/WebAssembly/eh-lsda.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ try.cont: ; preds = %entry, %catch.start

; CHECK-LABEL: test1:
; In static linking, we load GCC_except_table as a constant directly.
; NOPIC: i[[PTR]].const $push[[CONTEXT:.*]]=, __wasm_lpad_context
; NOPIC: i[[PTR]].const $push[[CONTEXT:.*]]=, {{[48]}}
; NOPIC-NEXT: i[[PTR]].const $push[[EXCEPT_TABLE:.*]]=, GCC_except_table1
; NOPIC-NEXT: i[[PTR]].store {{[48]}}($pop[[CONTEXT]]), $pop[[EXCEPT_TABLE]]
; NOPIC-NEXT: i[[PTR]].store __wasm_lpad_context($pop[[CONTEXT]]), $pop[[EXCEPT_TABLE]]
Copy link
Member

@dschuff dschuff Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting that this transformation eagerly swaps these 2 constants, even though it's not actually an optimization. I'm a little bit ambivalent about this because the original actually looks more natural to me (i.e. the operand contains the actual address, and the offset field is actually an offset). OTOH adding more complexity to the transform to prevent this doesn't seem great either, considering that it's not really any worse.

It does suggest though that we might be able to do a further optimization of actually combining these constants in the compiler? In an isolated case like this we couldn't actually remove the i32.const entirely (since there still needs to be an operand), and the constant offset field is always there, so it would potentially be of no benefit. But if we combine them into the offset and leave behind an i32.const 0, it's plausible that some subsequent optimization could remove it where appropriate. And this is a point where we have the information that the add is nuw so it's legal to do, whereas some subsequent pass (or post-link optimizer like Binaryen) might not have it. So it still seems worthwhile.

Copy link
Member

@dschuff dschuff Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose if we want to be really galaxy-brained, it's possible to have a situation where splitting an LEB-encoded value into 2 can actually reduce the overall size because of the way the encoding works; e.g. if one value is 128 and the other is 0, it takes 3 bytes to encode the first and one for the second, but if we split them into 2 64s, they would encode in 1 byte each!

... still, that seems like a bit of a stretch and maybe not as good as just leaving the 0 in the const field.

Copy link
Member

@dschuff dschuff Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait... actually we can't do this when one of the operands is relocatable anyway. So it doesn't really apply for this PR. And for cases where they are just regular constants, it seems likely they'd have already been combined earlier in the compiler pipeline anyway. So, yeah there's probably nothing there, other than my original observation about eagerly swapping the operands when there's no actual add instruction to eliminate.

Copy link
Contributor

@lukel97 lukel97 Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting that this transformation eagerly swaps these 2 constants, even though it's not actually an optimization. I'm a little bit ambivalent about this because the original actually looks more natural to me

FWIW I had the same thought too. I guess we could just swap around the offset and addrs and see if that maintains the original order in this test? But it's probably dependent on the order of the operands in the add node.

Copy link
Contributor Author

@badumbatish badumbatish Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried swapping Addr and Offset in WebAssemblyISelDAGToDAG and it seems it generates bad machine code on several test case, for example: cfg-stackify-eh.ll

*** Bad machine code: Expected a register operand. ***
- function:    two_catches
- basic block: %bb.1 catch.start (0x149811920)
- instruction: STORE_I32_A32 2, killed %3:i32, @__wasm_lpad_context, killed %2:i32, implicit-def dead $arguments :: (store (s32) into `ptr getelementptr inbounds ({ i32, ptr, i32 }, ptr @__wasm_lpad_context, i32 0, i32 1)`)
- operand 2:   @__wasm_lpad_context

*** Bad machine code: Expected a register operand. ***
- function:    two_catches
- basic block: %bb.1 catch.start (0x149811920)
- instruction: %1:i32 = LOAD_I32_A32 2, killed %6:i32, @__wasm_lpad_context, implicit-def dead $arguments :: (dereferenceable load (s32) from `ptr getelementptr inbounds ({ i32, ptr, i32 }, ptr @__wasm_lpad_context, i32 0, i32 2)`)
- operand 3:   @__wasm_lpad_context
LLVM ERROR: Found 2 machine code errors.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

Copy link
Contributor Author

@badumbatish badumbatish Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll push the changes to current nits (excluding swapping) but lmk if we'd like to pursue this avenue


; In case of PIC, we make GCC_except_table symbols a relative on based on
; __memory_base.
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/CodeGen/WebAssembly/exception-legacy.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ define void @throw(ptr %p) {
; CHECK: call foo
; CHECK: catch $[[EXN:[0-9]+]]=, __cpp_exception
; CHECK: global.set __stack_pointer
; CHECK: i32.{{store|const}} {{.*}} __wasm_lpad_context
; CHECK: i32.store __wasm_lpad_context
; CHECK: call $drop=, _Unwind_CallPersonality, $[[EXN]]
; CHECK: block
; CHECK: br_if 0
Expand Down
10 changes: 10 additions & 0 deletions llvm/test/CodeGen/WebAssembly/offset.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,16 @@ define i32 @load_i32_from_global_address() {
ret i32 %t
}

define i32 @load_i32_global_with_folded_gep_offset_nonconst_nuw(i32 %idx) {
; CHECK-LABEL: load_i32_global_with_folded_gep_offset_nonconst_nuw:
; CHECK: i32.const $push0=, 2
; CHECK: i32.shl $push1=, $0, $pop0
; CHECK: i32.load $push2=, gv($pop1)
%s = getelementptr nuw i32, ptr @gv, i32 %idx
%t = load i32, ptr %s
ret i32 %t
}

;===----------------------------------------------------------------------------
; Loads: 64-bit
;===----------------------------------------------------------------------------
Expand Down
Loading