[WebAssembly] Explicitly set `getCmpLibcallReturnType` to i32 by tgross35 · Pull Request #192425 · llvm/llvm-project

tgross35 · 2026-04-16T10:33:52Z

LLVM's default for getCmpLibcallReturnType is currently set to i32,
but this is not exactly accurate: the default in GCC is an integer of
word size. This means that on wasm64 targets, runtime libraries that are
assuming word size are incorrect and instead need to be using a 32-bit
integer.

The unintentional i32 default is actually a reasonable choice for Wasm
because it may be slightly cheaper to operate on when running 64-bit
wasm on 32-bit hosts, and there is no advantage to using a larger
integer (the result of comparisons need only be able to represent three
possible values). Thus, make the i32 explicit.

GCC does not support wasm64 so there is no compatibility concern at the
current time. If added, it would be reasonable for i32 to be used
there as well.

llvmbot · 2026-04-16T10:34:25Z

@llvm/pr-subscribers-backend-webassembly

Author: Trevor Gross (tgross35)

Changes

LLVM's default for getCmpLibcallReturnType is currently set to i32, but this is not exactly accurate: the default in GCC is an integer of word size. This means that on wasm64 targets, runtime libraries that are assuming word size are incorrect and instead need to be using a 32-bit integer.

The unintentional i32 default is actually a reasonable choice for Wasm because it may be slightly cheaper to operate on when running 64-bit wasm on 32-bit hosts, and there is no advantage to using a larger integer (the result of comparisons need only be able to represent three possible values). Thus, do the following:

Make getCmpLibcallReturnType explicit as i32 to prevent against future changes to the default.
Update compiler-rt to account for this and fix wasm64.
Note this decision in the changelog so other runtime libraries can update if needed.

GCC does not support wasm64 so there is no compatibility concern at the current time. If added, it would be reasonable for i32 to be used there as well.

Closes: #75302
Closes: #192416

Full diff: https://github.com/llvm/llvm-project/pull/192425.diff

3 Files Affected:

(modified) compiler-rt/lib/builtins/fp_compare_impl.inc (+3)
(modified) llvm/docs/ReleaseNotes.md (+4-1)
(modified) llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h (+4)

diff --git a/compiler-rt/lib/builtins/fp_compare_impl.inc b/compiler-rt/lib/builtins/fp_compare_impl.inc
index f883338c471d3..129d0136a0b98 100644
--- a/compiler-rt/lib/builtins/fp_compare_impl.inc
+++ b/compiler-rt/lib/builtins/fp_compare_impl.inc
@@ -15,6 +15,9 @@
 #if defined(__aarch64__) || defined(__arm64ec__)
 // AArch64 GCC overrides libgcc_cmp_return to use int instead of long.
 typedef int CMP_RESULT;
+#elif defined(__wasm__)
+// Both wasm32 and wasm64 use i32
+typedef int CMP_RESULT;
 #elif __SIZEOF_POINTER__ == 8 && __SIZEOF_LONG__ == 4
 // LLP64 ABIs use long long instead of long.
 typedef long long CMP_RESULT;
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index 9caeef89d0b1c..14958f5674d46 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -106,7 +106,7 @@ Changes to LLVM infrastructure
   ([#177046](https://github.com/llvm/llvm-project/pull/125687)). Note that
   there are still issues with invalid compiler reasoning about some functions
   in bitcode, e.g. `malloc`. Not yet supported on MachO or when using
-  distributed ThinLTO. 
+  distributed ThinLTO.
 
 Changes to building LLVM
 ------------------------
@@ -198,6 +198,9 @@ Changes to the RISC-V Backend
 Changes to the WebAssembly Backend
 ----------------------------------
 
+* The result of runtime library call comparisons is explicitly set to `i32` on
+  both wasm32 and wasm64.
+
 Changes to the Windows Target
 -----------------------------
 
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
index 42f047840e504..5de57615dccd4 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
@@ -29,6 +29,10 @@ class WebAssemblyTargetLowering final : public TargetLowering {
   MVT getPointerTy(const DataLayout &DL, uint32_t AS = 0) const override;
   MVT getPointerMemTy(const DataLayout &DL, uint32_t AS = 0) const override;
 
+  MVT::SimpleValueType getCmpLibcallReturnType() const override {
+    // i32 may be more efficient than the default word size for wasm64 targets
+    return MVT::i32;
+  }
 private:
   /// Keep a pointer to the WebAssemblySubtarget around so that we can make the
   /// right decision when generating code for different targets.

github-actions · 2026-04-16T10:35:04Z

✅ With the latest revision this PR passed the C/C++ code formatter.

sbc100 · 2026-04-16T15:58:50Z


+  MVT::SimpleValueType getCmpLibcallReturnType() const override {
+    // i32 may be more efficient than the default word size for wasm64 targets
+    return MVT::i32;


But doesn't the base class already default to returning i32? i.e. this override isn't doing anything is it?

See:

llvm-project/llvm/lib/CodeGen/TargetLoweringBase.cpp

Lines 1962 to 1964 in 547197d

MVT::SimpleValueType TargetLoweringBase::getCmpLibcallReturnType() const {

return MVT::i32; // return the default value

}

Currently yes but that's something that should change. This is just making the wasm choice explicit before that happens since (afaik) wasm64 is the only one to hit the case where i32 and a word have an incompatible ABI, aside from those with overrides in GCC.

I'm planning to follow up with #192441

I would rather you include the override when you land #192441.. since that is the point at which it becomes needed.

Also, I'm not actually sure its worth diverging here for wasm64. We might just want to go with wordsize too for consistency.

I can drop the override.

Also, I'm not actually sure its worth diverging here for wasm64. We might just want to go with wordsize too for consistency.

On most 64-bit targets there is no extra cost between a word sized comparison and a 32-bit and they share the same ABI, so those targets that happen to be consistent also aren't paying anything extra. IIUC however, wasm64 can still be running on 32-bit targets. On those, using a 64-bit comparison result rather than 32-bit means each libcall cmp costs up to (1) an extra register to be clobbered during return, (2) an extra instruction to put a meaningless value in that register, (3) ~2 extra instructions when checking the compare result.

This probably doesn't show up noticeably, especially considering the libcall is doing a lot more. But it seems like a reasonable enough case to break consistency considering 62 of the bits are wasted anyway.

sbc100

Any idea why this hasn't shown up already, for example, in emscripten?

tgross35 · 2026-04-16T16:11:32Z

Any idea why this hasn't shown up already, for example, in emscripten?

@Spxg pointed out in rust-lang/compiler-builtins#1199 that emscripten's fork of compiler-rt is patching for this https://github.com/emscripten-core/emscripten/blob/88e0002141254cdb772b558f63d2c1ad9b215eb2/system/lib/compiler-rt/lib/builtins/fp_compare_impl.inc#L15.

sbc100 · 2026-04-16T16:14:11Z

Any idea why this hasn't shown up already, for example, in emscripten?

@Spxg pointed out in rust-lang/compiler-builtins#1199 that emscripten's fork of compiler-rt is patching for this https://github.com/emscripten-core/emscripten/blob/88e0002141254cdb772b558f63d2c1ad9b215eb2/system/lib/compiler-rt/lib/builtins/fp_compare_impl.inc#L15.

Oh yes that makes sense :)

How about just porting emscripten patch here (i.e. just modifying line 15 instead of adding a whole new block?)

tgross35 · 2026-04-16T16:21:04Z

How about just porting emscripten patch here (i.e. just modifying line 15 instead of adding a whole new block?)

Mostly because "// AArch64 GCC overrides libgcc_cmp_return to use int instead of long." doesn't apply here

sbc100 · 2026-04-16T16:34:41Z

How about just porting emscripten patch here (i.e. just modifying line 15 instead of adding a whole new block?)

Mostly because "// AArch64 GCC overrides libgcc_cmp_return to use int instead of long." doesn't apply here

You can add anther line to the existing comment.

…method. NFC (#192483) This trivial method makes more sense in the header (indeed, both the existing overrides for it are already in headers). I noticed this while hunting for the default value while reviewing #192425

dschuff · 2026-04-20T20:55:59Z

/cc @aheejin

dschuff

I have a mild preference for the separate ifdef for wasm in compiler-rt since we have a separate comment, but I could go either way.

…method. NFC (llvm#192483) This trivial method makes more sense in the header (indeed, both the existing overrides for it are already in headers). I noticed this while hunting for the default value while reviewing llvm#192425

tgross35 · 2026-04-24T23:39:06Z

Thanks for the reviews. I'll need one of you to merge this, unless anyone feels more strongly about the comment or deferring the override. (I have a mild preference as-is of course, but 🤷‍♂️ )

sbc100 · 2026-04-24T23:43:13Z

I thought we were doing to avoid adding an override until one is needed? I.e. until #192441 lands?

sbc100 · 2026-04-24T23:43:51Z

 ----------------------------------

+* The result of runtime library call comparisons is explicitly set to `i32` on
+  both wasm32 and wasm64.


Since i32 was the default already I don't think we need to changelog entry do we? i.e. nothing actually changed here?

There is no change, but this was meant as an FYI that forks of compiler-rt or independent implementations may want to update. That affects a very small number of people though, so I'll drop.

sbc100 · 2026-04-24T23:45:12Z

 typedef int CMP_RESULT;
+#elif defined(__wasm__)
+// Both wasm32 and wasm64 use i32
+typedef int CMP_RESULT;


This change seems like it can land in isolation, it just reflects the current reality, right?

tgross35 · 2026-04-25T00:12:30Z

I thought we were doing to avoid adding an override until one is needed? I.e. until #192441 lands?

I still feel like having a target-specific change is nicer in log/blame to make it clear that this was an intentional decision, rather than being wrapped in a more generic "change default, preserve current behavior" diff. I've been bitten by this before, still wondering about some GCC ABI decisions that look like this in git :)

Also took the other approvals to think this was fine. But if you still feel strongly, will drop.

tgross35 · 2026-04-25T00:27:50Z

I think it's easier to just split off the compiler-rt changes since the context is pretty different. Opened #194093

LLVM's default for `getCmpLibcallReturnType` is currently set to `i32`, but this is not exactly accurate: the default in GCC is an integer of word size. This means that on wasm64 targets, runtime libraries that are assuming word size are incorrect and instead need to be using a 32-bit integer. The unintentional i32 default is actually a reasonable choice for Wasm because it may be slightly cheaper to operate on when running 64-bit wasm on 32-bit hosts, and there is no advantage to using a larger integer (the result of comparisons need only be able to represent three possible values). Thus, make the `i32` explicit. GCC does not support wasm64 so there is no compatibility concern at the current time. If added, it would be reasonable for `i32` to be used there as well.

…method. NFC (llvm#192483) This trivial method makes more sense in the header (indeed, both the existing overrides for it are already in headers). I noticed this while hunting for the default value while reviewing llvm#192425

tgross35 mentioned this pull request Apr 16, 2026

Mismatched ABI for long double comparison builtins on wasm64-unknown-unknown #192416

Closed

llvmbot added compiler-rt backend:WebAssembly compiler-rt:builtins labels Apr 16, 2026

tgross35 force-pushed the wasm-cmp-update branch from 64928b1 to e45bb7d Compare April 16, 2026 10:37

tgross35 mentioned this pull request Apr 16, 2026

[TargetLowering] Set the default getCmpLibcallReturnType to word size #192441

Open

sbc100 reviewed Apr 16, 2026

View reviewed changes

sbc100 mentioned this pull request Apr 16, 2026

[CodeGen] Inline trivial TargetLoweringBase::getCmpLibcallReturnType method. NFC #192483

Merged

dschuff approved these changes Apr 20, 2026

View reviewed changes

aheejin approved these changes Apr 21, 2026

View reviewed changes

tgross35 force-pushed the wasm-cmp-update branch from e45bb7d to ebae57a Compare April 24, 2026 23:37

sbc100 reviewed Apr 24, 2026

View reviewed changes

tgross35 mentioned this pull request Apr 25, 2026

[compiler-rt][WebAssembly] Use an int as CMP_RESULT #194093

Merged

tgross35 force-pushed the wasm-cmp-update branch from ebae57a to 385a4b1 Compare April 25, 2026 03:11

	MVT::SimpleValueType TargetLoweringBase::getCmpLibcallReturnType() const {
	return MVT::i32; // return the default value
	}

Conversation

tgross35 commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 16, 2026

Uh oh!

github-actions Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Apr 16, 2026

Uh oh!

sbc100 commented Apr 16, 2026

Uh oh!

tgross35 commented Apr 16, 2026

Uh oh!

sbc100 commented Apr 16, 2026

Uh oh!

dschuff commented Apr 20, 2026

Uh oh!

dschuff left a comment

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Apr 24, 2026

Uh oh!

sbc100 commented Apr 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Apr 25, 2026

Uh oh!

tgross35 commented Apr 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tgross35 commented Apr 16, 2026 •

edited

Loading

github-actions Bot commented Apr 16, 2026 •

edited

Loading