Add nontrapping-fptoint lowering pass #7016

dschuff · 2024-10-17T18:46:58Z

This pass lowers nontrapping FP to int instructions to implement LLVM's conversion behavior.
This means that they are not fully complete lowerings according to the wasm spec, but have the same
undefined behavior that LLM does. This keeps the pass simpler and preserves existing behavior when
compiling without nontrapping-ft.
This will be used in emscripten, so that we can build libraries with nontrapping-fp and lower them away after link if desired.

src/passes/LLVMNontrappingFPToIntLowering.cpp

test/lit/passes/nontrapping-fptoint-lowering.wast

…update test

dschuff · 2024-11-15T00:03:48Z

OK, I added an exec test similar to what we have for memcpy. I think this should be good to review now.

kripken · 2024-11-15T00:14:37Z

src/passes/CMakeLists.txt

@@ -76,6 +76,7 @@ set(passes_SOURCES
  NameList.cpp
  NameTypes.cpp
  NoInline.cpp
+  LLVMNontrappingFPToIntLowering.cpp


We didn't put LLVM in the name of the bulk memory pass IIRC - should we match the two? I don't feel strongly either way.

Ah yeah my original intention was that it should lower just memory.copy and memory.fill (not all of bulk memory) but that part wouldn't need to be LLVM-specific. But then I made it have the same undefined behavior LLVM does, so I think it does make sense to put LLVM in the name. I'll do that in a separate PR.

Done in #7082

src/passes/LLVMNontrappingFPToIntLowering.cpp

kripken · 2024-11-15T00:18:52Z

test/lit/exec/nontrapping-fptoint-lowering.wat

+  (func $i64.trunc_sat_f64_s (param $x f64) (result i64) (i64.trunc_sat_f64_s (local.get $x)))
+  (func $i64.trunc_sat_f64_u (param $x f64) (result i64) (i64.trunc_sat_f64_u (local.get $x)))
+
+  (func $f32_i32 (export "f32_i32")


The CHECK lines don't seem to be here? May need to run ./scripts/update_lit_checks.py test/lit/exec/*

ah oops, yes, the fuzz comparison runs, but the test run was written not to generate output. I think I pasted that from somewhere, I guess some tests want to check the output and some don't.

Actually, does it make more sense just to not print the wast output in this test? test/lit/passes/nontrapping-fptoint-lowering.wast prints the output, and the output from this test is the same.

The wast isn't really needed, I agree. But we should still have the execution output, even if just to report that the function was called and returned nothing.

Actually, I see --fuzz-exec is missing from line 3, is that the issue maybe?

Yes, that was why the tests didn't get auto-updated. Fixed now, using just the fuzz-exec output.

test/lit/passes/nontrapping-fptoint-lowering.wast

dschuff · 2024-11-15T17:43:02Z

@aheejin can you PTAL, @kripken is out today.

aheejin · 2024-11-16T07:19:27Z

src/passes/LLVMNontrappingFPToIntLowering.cpp

+      case TruncSatSFloat32ToInt32:
+        return UnaryOp::TruncSFloat32ToInt32;


Nit: It'd be better to be consistent whether or not we use UnaryOp::? The same for other parts of the code.

aheejin · 2024-11-16T07:47:55Z

src/passes/LLVMNontrappingFPToIntLowering.cpp

+      case TruncSatUFloat32ToInt32:
+        ltOp = BinaryOp::LtFloat32;
+        geOp = BinaryOp::GeFloat32;
+        break;
+      case TruncSatUFloat64ToInt32:
+        ltOp = BinaryOp::LtFloat64;
+        geOp = BinaryOp::GeFloat64;
+        break;
+      case TruncSatUFloat32ToInt64:
+        ltOp = BinaryOp::LtFloat32;
+        geOp = BinaryOp::GeFloat32;
+        break;
+      case TruncSatUFloat64ToInt64:
+        ltOp = BinaryOp::LtFloat64;
+        geOp = BinaryOp::GeFloat64;
+        break;


Suggested change

case TruncSatUFloat32ToInt32:

ltOp = BinaryOp::LtFloat32;

geOp = BinaryOp::GeFloat32;

break;

case TruncSatUFloat64ToInt32:

ltOp = BinaryOp::LtFloat64;

geOp = BinaryOp::GeFloat64;

break;

case TruncSatUFloat32ToInt64:

ltOp = BinaryOp::LtFloat32;

geOp = BinaryOp::GeFloat32;

break;

case TruncSatUFloat64ToInt64:

ltOp = BinaryOp::LtFloat64;

geOp = BinaryOp::GeFloat64;

break;

case TruncSatUFloat32ToInt32:

case TruncSatUFloat32ToInt64:

ltOp = BinaryOp::LtFloat32;

geOp = BinaryOp::GeFloat32;

break;

case TruncSatUFloat64ToInt32:

ltOp = BinaryOp::LtFloat64;

geOp = BinaryOp::GeFloat64;

break;

aheejin · 2024-11-16T07:50:04Z

src/passes/pass.cpp

@@ -334,6 +334,10 @@ void PassRegistry::registerPasses() {
  registerPass("no-partial-inline",
               "mark functions as no-inline (for partial inlining only)",
               createNoPartialInlinePass);
+  registerPass("llvm-nontrapping-fptoint-lowering",
+               "lower nontrapping float-to-int operations to wasm mvp and"


Suggested change

"lower nontrapping float-to-int operations to wasm mvp and"

"lower nontrapping float-to-int operations to wasm mvp and "

aheejin · 2024-11-16T07:50:16Z

test/lit/help/wasm-metadce.test

@@ -230,6 +230,11 @@
 ;; CHECK-NEXT:   --limit-segments                              attempt to merge segments to fit
 ;; CHECK-NEXT:                                                 within web limits
 ;; CHECK-NEXT:
+;; CHECK-NEXT:   --llvm-nontrapping-fptoint-lowering           lower nontrapping float-to-int
+;; CHECK-NEXT:                                                 operations to wasm mvp
+;; CHECK-NEXT:                                                 anddisable the nontrapping


Suggested change

;; CHECK-NEXT: anddisable the nontrapping

;; CHECK-NEXT: and disable the nontrapping

aheejin · 2024-11-16T07:50:25Z

test/lit/help/wasm-opt.test

@@ -239,6 +239,11 @@
 ;; CHECK-NEXT:   --limit-segments                              attempt to merge segments to fit
 ;; CHECK-NEXT:                                                 within web limits
 ;; CHECK-NEXT:
+;; CHECK-NEXT:   --llvm-nontrapping-fptoint-lowering           lower nontrapping float-to-int
+;; CHECK-NEXT:                                                 operations to wasm mvp
+;; CHECK-NEXT:                                                 anddisable the nontrapping


Suggested change

;; CHECK-NEXT: anddisable the nontrapping

;; CHECK-NEXT: and disable the nontrapping

aheejin · 2024-11-16T07:50:32Z

test/lit/help/wasm2js.test

@@ -193,6 +193,11 @@
 ;; CHECK-NEXT:   --limit-segments                              attempt to merge segments to fit
 ;; CHECK-NEXT:                                                 within web limits
 ;; CHECK-NEXT:
+;; CHECK-NEXT:   --llvm-nontrapping-fptoint-lowering           lower nontrapping float-to-int
+;; CHECK-NEXT:                                                 operations to wasm mvp
+;; CHECK-NEXT:                                                 anddisable the nontrapping


Suggested change

;; CHECK-NEXT: anddisable the nontrapping

;; CHECK-NEXT: and disable the nontrapping

aheejin · 2024-11-16T08:11:22Z

src/passes/LLVMNontrappingFPToIntLowering.cpp

+        builder.makeConst(static_cast<From>(std::numeric_limits<To>::max()))),
+      builder.makeUnary(getReplacementOp(curr->op),
+                        builder.makeLocalGet(v, curr->value->type)),
+      builder.makeConst(std::numeric_limits<To>::min())));


Are this value and '0' in replaceUnsigned arbitrarily chosen?
(LLVM's choices are different (but it looks they are arbitrarily chosen too): https://github.com/llvm/llvm-project/blob/2906fcadb8563a02949f852867cebc63e0539cb7/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp#L492)

And these values don't seem to conform to the real "saturating" spec, because we don't check whether the value overflows or underflows and just use a single substitute value. Is this the reason why this pass is LLVM-specific? Then what is the reason for not making pass a generic conversion pass by implementing the real saturation behavior? (I guess performance?)

aheejin · 2024-11-16T08:25:17Z

src/passes/LLVMNontrappingFPToIntLowering.cpp

+        ltOp,
+        builder.makeUnary(
+          absOp, builder.makeLocalTee(v, curr->value, curr->value->type)),
+        builder.makeConst(static_cast<From>(std::numeric_limits<To>::max()))),


Separate question: Do you know why LLVM's code adds -(minus)?
https://github.com/llvm/llvm-project/blob/2906fcadb8563a02949f852867cebc63e0539cb7/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp#L493

Also, does this handle when the input value is greater than INT_MIN and less than or equal to INT_MIN+1? INT_MIN's abs value would be greater than INT_MAX but it's still in the int range. (in case of int)

dschuff added 6 commits October 16, 2024 09:25

Works for 32->32

be05442

Templatize

90bd6d5

hacks to run it

ea01454

unreachable

7df947b

remove debug code

f3eff06

update help checks

9810d17

dschuff commented Oct 17, 2024

View reviewed changes

src/passes/LLVMNontrappingFPToIntLowering.cpp Outdated Show resolved Hide resolved

dschuff requested a review from kripken October 17, 2024 18:50

kripken reviewed Oct 17, 2024

View reviewed changes

src/passes/LLVMNontrappingFPToIntLowering.cpp Outdated Show resolved Hide resolved

dschuff added 3 commits October 17, 2024 15:18

tee into a local

d1db88e

add more explanation

925d34a

remove debugging code

07dd028

kripken reviewed Oct 17, 2024

View reviewed changes

dschuff added 17 commits October 17, 2024 16:23

formatting

ab7431a

clang-format

d3ae179

Lower memory.copy

0ed5b59

update help tests

9bbd758

reverse loop condition

16285bb

add memory fill

47dc98f

copy in both directions, add var names

c373c13

Fix bugs

46aaa06

use Fatal instead of throw; error on other bulk-memory instructions; …

8c74874

…update test

review

d0f3c87

clang-format

914def0

fix func names

c758b5f

improve formatting

02201ef

suppress clang-format

2854ff3

revert formatting changes; not worth it

3ccb64f

Merge branch 'copy-fill-lowering' into nontrapping-lower

573275f

add test

8eaebf6

Merge branch 'main' into nontrapping-lower

6f29023

kripken reviewed Nov 15, 2024

View reviewed changes

dschuff added 2 commits November 15, 2024 09:26

update test

d735093

convert to function-parallel pass

f6483d1

dschuff requested a review from aheejin November 15, 2024 17:42

clang-format

e7c50cf

aheejin reviewed Nov 16, 2024

View reviewed changes

aheejin mentioned this pull request Nov 16, 2024

Rename memory-copy-fill-lowering pass #7082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nontrapping-fptoint lowering pass #7016

Add nontrapping-fptoint lowering pass #7016

dschuff commented Oct 17, 2024 •

edited

Loading

dschuff commented Nov 15, 2024

kripken Nov 15, 2024

dschuff Nov 15, 2024

dschuff Nov 15, 2024

kripken Nov 15, 2024

dschuff Nov 15, 2024

dschuff Nov 15, 2024

kripken Nov 15, 2024

dschuff Nov 15, 2024

dschuff commented Nov 15, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

aheejin Nov 16, 2024

		case TruncSatSFloat32ToInt32:
		return UnaryOp::TruncSFloat32ToInt32;

	"lower nontrapping float-to-int operations to wasm mvp and"
	"lower nontrapping float-to-int operations to wasm mvp and "

	;; CHECK-NEXT: anddisable the nontrapping
	;; CHECK-NEXT: and disable the nontrapping

Add nontrapping-fptoint lowering pass #7016

Are you sure you want to change the base?

Add nontrapping-fptoint lowering pass #7016

Conversation

dschuff commented Oct 17, 2024 • edited Loading

dschuff commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Oct 17, 2024 •

edited

Loading