Fix sse tests with -mnontrapping-fptoint
#22893
Conversation
sbc100
force-pushed
the
nontrapping_sse
branch
2 times, most recently
from
ffd4ddf to
5003169
Compare
| @no_asan('local count too large') | ||
| def test_sse2(self): | ||
| @parameterized({ | ||
| '': ([],), |
There was a problem hiding this comment.
let's make this an explicit `-mno-nontrapping-fptoint' so that when we flip the default, it will still be testing the same case.
There was a problem hiding this comment.
I think maybe better to explicitly change these tests when we make that change, since the variant will want to be renamed so the default is still empty string and the non-default becomes perhaps -trapping? Or -no-nontrapping?
There was a problem hiding this comment.
or we could just name both variants I guess.
There was a problem hiding this comment.
I'm kind keen on the idea of always having an unnamed variant for the default. Then there is always a test you can run that matches the name of the test in python. i.e. if I see def test_foo_bar i know I can run a test called test_foo_bar without any suffix. Maybe no a necessarily things, but it seems nice to have.
There was a problem hiding this comment.
We already have other tests here like test_fasta_nontrapping that will need to be inverted when we change the default.
| @no_ubsan('test contains UB') | ||
| def test_sse1(self): | ||
| @parameterized({ | ||
| '': ([],), |
Turns out we were relying on the explicit llvm bounds check here to give use the defined SSE behavior, but normal C semantics don't give us that so we need explicit bounds checks here.
sbc100
force-pushed
the
nontrapping_sse
branch
from
5003169 to
801ee20
Compare
dschuff
left a comment
dschuff
left a comment
There was a problem hiding this comment.
This seems fine either way.
There does still seem to be a fair amount of opportunity to optimize in here. But I guess there are also plenty of //TODO: optimize so no change :)
Turns out we were relying on the explicit llvm bounds check here to give use the defined SSE behavior, but normal C semantics don't give us that so we need explicit bounds checks here. Fixes emscripten-core#22892
2 participants
Turns out we were relying on the explicit llvm bounds check here to give use the defined SSE behavior, but normal C semantics don't give us that so we need explicit bounds checks here.
Fixes #22892