This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix invalid machine instrs in -O0, verify in tests
ClosedPublic

Authored by tlively on Dec 20 2018, 1:58 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Dec 20 2018, 1:58 PM
aheejin added inline comments.Dec 20 2018, 2:49 PM
test/CodeGen/WebAssembly/simd-noopt.ll
9 ↗(On Diff #179152)

No check lines?

tlively marked an inline comment as done.Dec 20 2018, 2:51 PM
tlively added inline comments.
test/CodeGen/WebAssembly/simd-noopt.ll
9 ↗(On Diff #179152)

I'm mostly just interested in this not crashing, but would be happy to add some if you think it would be better.

aheejin added inline comments.Dec 20 2018, 2:52 PM
test/CodeGen/WebAssembly/simd-noopt.ll
9 ↗(On Diff #179152)

Not crashing meaning, not crashing llvm regression test or V8? If you mean the latter, I guess it's better to check that v2i64 types are not generated by one or two CHECK-NOT lines.

tlively marked an inline comment as done.Dec 20 2018, 2:55 PM
tlively added inline comments.
test/CodeGen/WebAssembly/simd-noopt.ll
9 ↗(On Diff #179152)

Oh no, sorry, I meant not crashing LLVM. It was previously crashing because it was generating MachineInstrs that used a register that had never been defined.

aheejin accepted this revision.Dec 20 2018, 2:59 PM

LGTM with a nit

test/CodeGen/WebAssembly/simd-noopt.ll
9 ↗(On Diff #179152)

Then I guess it's better add a comment to this test what this is supposed to test, because if someone does not know the context that some types are enabled only with a flag (which is not included in this case) and this test is supposed to test the bahavior of *not* generating those types in case of -O0.

This revision is now accepted and ready to land.Dec 20 2018, 2:59 PM
aheejin added inline comments.Dec 20 2018, 3:00 PM
test/CodeGen/WebAssembly/simd-noopt.ll
9 ↗(On Diff #179152)

Sorry the comment ended prematurely, what I wanted to write is "because if someone does not know ..., it would be hard to understand what this test is doing."

This revision was automatically updated to reflect the committed changes.

Sorry for nits after the commit :(

llvm/trunk/test/CodeGen/WebAssembly/simd-noopt.ll
6

Sorry for the after-comment: Could you specify that this is a stop-gap thing that's effective only until v2i64 is implemented? Not sure if it is clear why this simd test does not produce simd instructions at all, if someone doesn't know the history of -wasm-enable-unimplemented-simd. (This is what I meant in the first place, sorry if I wasn't clear)

9

no need of -wasm at the end

20

You can delete this and add -fast-isel to the command line

tlively marked 3 inline comments as done.Jan 2 2019, 12:47 PM

Comments addressed in rL350260.