Add sign-ext lowering pass when target browser doesn't support it #17689

sbc100 · 2022-08-19T19:55:50Z

llvm and binaryen now default to enabling sign-ext and mutable-globals
by default. See https://reviews.llvm.org/D125728.

In order to allow users to continue to target older browsers we will
now lower sign-ext operations in a binaryen pass if the target browsers
don't support it.

sbc100 · 2022-08-19T19:57:53Z

This is really a WIP to show what we will likely have to do in preparation for the llvm-side change.

dschuff · 2022-08-19T20:57:48Z

As part of this change we are bumping the default minimum browser
versions for Safari and Firefox to the minimum version needed to
supported these 4 features which are about to be enabled by default:

should we do a no-op change first, which doesn't bump the versions?

Another question. I assume that if a user passes -mnontrapping-fptoint or -pthreads or -fwasm-exceptions or -mno-signext that it will have the usual effect, independently of this change?
(actually threads and EH are special since I guess they'll stay off by default anyway....) But in other words, if a user uses a clang flag it can override what emscripten thinks the default feature set is. this is fine if the feature is only enabled by clang, but if a feature also needs support in emscripten it gets more complicated...

sbc100 · 2022-08-19T21:07:40Z

Another question. I assume that if a user passes -mnontrapping-fptoint or -pthreads or -fwasm-exceptions or -mno-signext that it will have the usual effect, independently of this change?
(actually threads and EH are special since I guess they'll stay off by default anyway....) But in other words, if a user uses a clang flag it can override what emscripten thinks the default feature set is. this is fine if the feature is only enabled by clang, but if a feature also needs support in emscripten it gets more complicated...

Yes that works both before and after this change explicit user flags come later and therefore take precedence

tlively · 2022-08-19T21:20:07Z

New feature matrix code looks good to me 👍

sbc100 · 2022-08-19T21:32:12Z

I split out the actual bumping the default versions into #17690

sbc100 · 2022-08-19T21:45:22Z

Added a test. PTAL

dschuff

I guess we could also ask the opposite question of "what if the user only passes the feature flags at link time and doesn't at compile time": I guess in that case things would also work as they do now... wasm features would be unaffected, but JS/web features would be adjusted accordingly.

tools/feature_matrix.py

tools/settings.py

kripken · 2022-08-22T21:28:38Z

test/test_other.py

+    self.assertNotContained('nontrapping-fptoint', enabled)
+    self.assertNotContained('mutable-globals', enabled)
+
+    # If we not disable all support for Edge/Safari/Firefox those feature should now be


Suggested change

# If we not disable all support for Edge/Safari/Firefox those feature should now be

# If we disable all support for Edge/Safari/Firefox those feature should now be

test/test_other.py

kripken · 2022-08-22T21:29:29Z

test/test_other.py

+    self.run_process([EMCC, '-mcpu=bleeding-edge', '-c', test_file('hello_world.c'),
+                      '-sMIN_EDGE_VERSION=-1',
+                      '-sMIN_SAFARI_VERSION=-1',
+                      '-sMIN_FIREFOX_VERSION=-1'])


Why is Chrome different here?

Because its the default values of the other browsers that prevent those features being enabled. The default chrome version is good enough apparently. I updated the comment.

kripken

Please mention this in the changelog. Would be good to mention both directions - both that we disable as needed, and enable based on pthreads and bulk memory now.

test/test_other.py

tools/feature_matrix.py

sbc100 · 2022-11-15T20:06:52Z

We ended up landing on a much simpler solution that doesn't involve CFLAGS or llvm flags. We simply lower sign-ext operations away in a binaryen pass if user is targeting an older browser.

As a followup we plan to update the default browser versions such that sign-ext is support by default.

kripken

lgtm % comments

emcc.py

test/test_other.py

tools/feature_matrix.py

emcc.py

test/test_other.py

juj · 2022-11-16T11:07:09Z

If I understand correctly:

LLVM still supports compiling code with both sign-ext enabled vs disabled, and mutable-globals enabled vs disabled. So they did not e.g. drop support altogether for compiling against sign-ext disabled for example?
The way this PR is laid out is that if user passes MIN_xxx_VERSION smaller than what is required for sign-ext, an extra Binaryen pass is run to lower the sign-ext code, and a log diagnostic message is printed to the debug channel that this happened. This Binaryen lowering pass occurs even if user had passed -mno-sign-ext to compile+link stages in the first place?

Do the Wasm object files record which features had been enabled when they were being built? That would allow quickly detecting if a lowering pass is needed at all?

sbc100 · 2022-11-16T15:47:59Z

If I understand correctly:

LLVM still supports compiling code with both sign-ext enabled vs disabled, and mutable-globals enabled vs disabled. So they did not e.g. drop support altogether for compiling against sign-ext disabled for example?

Correct.

The way this PR is laid out is that if user passes MIN_xxx_VERSION smaller than what is required for sign-ext, an extra Binaryen pass is run to lower the sign-ext code, and a log diagnostic message is printed to the debug channel that this happened. This Binaryen lowering pass occurs even if user had passed -mno-sign-ext to compile+link stages in the first place?

Correct. The lowering pass happens when you target older browsers, regardless of whether -mno-sign-ext was used to try to avoid the instruction.

One compelling reason for doing it this way is it ensuring that all input files are built with -mno-sign-ext isn't just about user code, it would require all system libraries to be rebuilt with this flag (i.e. adding a new system library variant)

Do the Wasm object files record which features had been enabled when they were being built? That would allow quickly detecting if a lowering pass is needed at all?

Yes, wasm object files do contain that information, and actually I think we could rely on the linker outputting the features needed in the final binary.

However, I'm not sure its worth doing this, since the running of this pass is only done if a non-default set of browsers are targeted. If such older browser are being targeted then is almost certain that -sWASM_BIGINT is also not being used, so binaryen is already required anyway, and adding this pass is basically free.

sbc100 · 2022-11-16T15:52:31Z

and a log diagnostic message is printed to the debug channel that this happened.

Do you think reporting this information is important? I don't think we report it when we perform i64 lowering, which happens in the default case (i.e. when -sWASM_BIGINT is not passed).

kripken · 2022-11-16T20:46:54Z

emcc.py

@@ -476,7 +477,6 @@ def default_min_browser_version(browser, version):

  if settings.WASM_BIGINT:
    default_min_browser_version('Safari', 150000)
-    default_min_browser_version('Edge', 79)


unrelated change?

Actually its needed, since setting the edge version to anything by -1 currently means no features are supported. i.e. the edge version currently means only EdgeHTML and not modern Edge.

I don't follow. First, why can't we use a single number for both browsers? I thought their version numbers did not overlap, and were monotonically increasing.

Second, what does it mean to not mention Edge here at all? Will anyone setting any value of Edge get sign-ext lowered out?

The proposal is that for now the edge version only refers to the old edge browser, and if the features of modern edge ever diverge from chromium we can start to support MIN_EDGE_VERSION being distinct from MIN_CHROME_VERSION

Not mentioning edge at all there just means that default of "-1" (not supported at all) will prevail.

Right now I'm not considering edge at all in the feature_matix.py because we currently assume that folks can use the MIN_CHROMIUM_VERSION to target modern versions of edge. Instead of removing this line I could instead extend feature_matrix.py to include support for MIN_EDGE_VERSION (but it would just be identical what is already there for chromium).

OK, I reworked feature_matrix.py such that this change is no longer needed.

Oh, then the Chrome version refers to modern Edge too? Sgtm, but I wasn't aware their features had even been in sync so far. Is that the case in practice?

If you looks at the versions in https://en.wikipedia.org/wiki/Microsoft_Edge (expand "New Edge release history") you will see that edge versions correspond 1-to-1 with blink versions.. to from a JS/wasm perspective one would expect them to always match.

llvm and binaryen now default to enabling sign-ext and mutable-globals by default. See https://reviews.llvm.org/D125728. In order to allow users to continue to target older browsers we will now lower sign-ext operations in a binaryen pass if the target browsers don't support it.

juj · 2022-11-17T12:41:59Z

it would require all system libraries to be rebuilt with this flag

Good point, forgot about that. Agree, we wouldn't want to build different variants of the system libs for each combination.

juj · 2022-11-17T12:45:13Z

If such older browser are being targeted then is almost certain that -sWASM_BIGINT is also not being used, so binaryen is already required anyway, and adding this pass is basically free.

Ah right, there's already other steps that are needed. Yeah sgtm like this.

sbc100 requested review from kripken, dschuff and tlively and removed request for kripken and dschuff August 19, 2022 19:57

sbc100 force-pushed the feature_matrix branch from f6ee99a to b4b569a Compare August 19, 2022 20:36

sbc100 force-pushed the feature_matrix branch from b4b569a to a4ce59e Compare August 19, 2022 21:31

sbc100 force-pushed the feature_matrix branch 2 times, most recently from bbe0aa0 to b408293 Compare August 19, 2022 21:43

sbc100 force-pushed the feature_matrix branch 2 times, most recently from 722187e to 0407a6b Compare August 19, 2022 23:23

dschuff reviewed Aug 20, 2022

View reviewed changes

tools/feature_matrix.py Outdated Show resolved Hide resolved

sbc100 mentioned this pull request Aug 20, 2022

Bump min browser versions such they all support llvm default features #17690

Merged

sbc100 requested a review from juj August 20, 2022 15:05

sbc100 force-pushed the feature_matrix branch from 0407a6b to 2b6ad46 Compare August 22, 2022 19:07

kripken reviewed Aug 22, 2022

View reviewed changes

sbc100 force-pushed the feature_matrix branch 2 times, most recently from 3fc9bc8 to 88cce67 Compare August 22, 2022 22:18

kripken reviewed Aug 24, 2022

View reviewed changes

test/test_other.py Outdated Show resolved Hide resolved

test/test_other.py Outdated Show resolved Hide resolved

tools/feature_matrix.py Outdated Show resolved Hide resolved

juj reviewed Aug 25, 2022

View reviewed changes

tools/feature_matrix.py Outdated Show resolved Hide resolved

sbc100 force-pushed the feature_matrix branch 2 times, most recently from 704cf80 to c534e46 Compare November 15, 2022 20:04

sbc100 requested review from juj and dschuff November 15, 2022 20:05

sbc100 force-pushed the feature_matrix branch 2 times, most recently from 5058ccd to c530c66 Compare November 15, 2022 20:08

kripken reviewed Nov 15, 2022

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

test/test_other.py Outdated Show resolved Hide resolved

tools/feature_matrix.py Show resolved Hide resolved

sbc100 force-pushed the feature_matrix branch from c530c66 to 5a9c009 Compare November 15, 2022 22:33

juj reviewed Nov 16, 2022

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

juj reviewed Nov 16, 2022

View reviewed changes

test/test_other.py Outdated Show resolved Hide resolved

sbc100 force-pushed the feature_matrix branch 4 times, most recently from d259b90 to 4a0c219 Compare November 16, 2022 20:32

sbc100 requested a review from kripken November 16, 2022 20:33

sbc100 force-pushed the feature_matrix branch from 4a0c219 to 00e136d Compare November 16, 2022 20:33

sbc100 requested a review from juj November 16, 2022 20:33

kripken approved these changes Nov 16, 2022

View reviewed changes

sbc100 force-pushed the feature_matrix branch from 00e136d to 66596c4 Compare November 16, 2022 21:58

sbc100 force-pushed the feature_matrix branch from 66596c4 to 95eb395 Compare November 16, 2022 22:13

kripken approved these changes Nov 16, 2022

View reviewed changes

sbc100 enabled auto-merge (squash) November 16, 2022 23:21

sbc100 merged commit 21509ee into main Nov 16, 2022

sbc100 deleted the feature_matrix branch November 16, 2022 23:31

sbc100 mentioned this pull request Jan 29, 2023

Fix compatibility with Emscripten 3.1.28 hoodmane/libffi-emscripten#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sign-ext lowering pass when target browser doesn't support it #17689

Add sign-ext lowering pass when target browser doesn't support it #17689

sbc100 commented Aug 19, 2022 •

edited

Loading

sbc100 commented Aug 19, 2022

dschuff commented Aug 19, 2022

sbc100 commented Aug 19, 2022

tlively commented Aug 19, 2022

sbc100 commented Aug 19, 2022

sbc100 commented Aug 19, 2022

dschuff left a comment

kripken Aug 22, 2022

kripken Aug 22, 2022

sbc100 Aug 22, 2022

kripken left a comment

sbc100 commented Nov 15, 2022

kripken left a comment

juj commented Nov 16, 2022

sbc100 commented Nov 16, 2022 •

edited

Loading

sbc100 commented Nov 16, 2022

kripken Nov 16, 2022

sbc100 Nov 16, 2022

kripken Nov 16, 2022

sbc100 Nov 16, 2022

sbc100 Nov 16, 2022

kripken Nov 16, 2022

sbc100 Nov 16, 2022

juj commented Nov 17, 2022

juj commented Nov 17, 2022

	# If we not disable all support for Edge/Safari/Firefox those feature should now be
	# If we disable all support for Edge/Safari/Firefox those feature should now be

Add sign-ext lowering pass when target browser doesn't support it #17689

Add sign-ext lowering pass when target browser doesn't support it #17689

Conversation

sbc100 commented Aug 19, 2022 • edited Loading

sbc100 commented Aug 19, 2022

dschuff commented Aug 19, 2022

sbc100 commented Aug 19, 2022

tlively commented Aug 19, 2022

sbc100 commented Aug 19, 2022

sbc100 commented Aug 19, 2022

dschuff left a 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

kripken left a comment

Choose a reason for hiding this comment

sbc100 commented Nov 15, 2022

kripken left a comment

Choose a reason for hiding this comment

juj commented Nov 16, 2022

sbc100 commented Nov 16, 2022 • edited Loading

sbc100 commented Nov 16, 2022

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

juj commented Nov 17, 2022

juj commented Nov 17, 2022

sbc100 commented Aug 19, 2022 •

edited

Loading

sbc100 commented Nov 16, 2022 •

edited

Loading