-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Description
From https://reviews.llvm.org/D139645:
Given the following C:
extern int *data;
int f(unsigned idx) {
return data[idx];
}
You would think we could emit
i32.load8_u data($0)
But instead we currently emit
h: # @h
.functype h (i32) -> (i32)
# %bb.0: # %entry
i64.const data
local.get 0
i64.extend_i32_u
i64.add
i32.load8_s 0
We can't naively fold the add into the load offset though, because the effective address calculation is unsigned, which can overflow with negative offsets:
OK, so looking back at #29497 and D24053 this actually does look like the problem that we fixed back then.
IIRC the problem is that sometimes the address operand of the store (i.e. local 0, or L1 here) can have a negative value (here it's -128), but the store's effective address calculation (i.e. the operand plus the constant offset, L1 + args + 128 with this CL applied) is unsigned, so it will overflow.
So we have to ensure that the calculation that recombines the native local value with the compile-time constant (here 128) happens with an add instruction rather than getting folded.
For signed types, this is a problem. But there should be a way to do the folding for unsigned types.
Activity
llvmbot commentedon Apr 4, 2023
@llvm/issue-subscribers-backend-webassembly
lukel97 commentedon May 5, 2024
@dschuff There's an RFC to add a nuw flag to GEP that might be relevant here: https://discourse.llvm.org/t/rfc-add-nusw-and-nuw-flags-for-getelementptr/. Clang could potentially use it to retain some of the information about the offset being unsigned
dschuff commentedon May 6, 2024
Oh, that looks very nice. Getting
nuw
in more places looks like just what we need to do more offset-folding.[WebAssembly] Fold TargetGlobalAddress with added offset (#145829)