This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][NFC] simplify SjLj inline assembly test
ClosedPublic

Authored by quantum on Jul 2 2019, 6:22 PM.

Details

Summary

Per feedback in D64115, simplify the test.

hidden is left in though, because every test in the file has it.

Event Timeline

quantum created this revision.Jul 2 2019, 6:22 PM
aheejin accepted this revision.Jul 2 2019, 6:27 PM

Hmm right, every other function has hidden... while you're on a roll, can you remove all hiddens here? :) Thanks!

This revision is now accepted and ready to land.Jul 2 2019, 6:27 PM
aheejin added inline comments.Jul 2 2019, 6:29 PM
llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
200–201

Oh one more thing. I think it's better to do
CHECK-NOT: invoke void blah blah
so that what this test is looking for. And maybe add a one-line comment that inline asms should NOT be transformed into invoke wrappers.

aheejin added inline comments.Jul 2 2019, 6:35 PM
llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
200–201

so that what this test is looking for -> so that it is clear what this is looking for

quantum marked an inline comment as done.Jul 3 2019, 10:31 AM
quantum added inline comments.
llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
200–201

If it generated an invoke wrapper, opt would exit with a failure. I'll put the CHECK-NOT in anyways to make it more explicit.

quantum updated this revision to Diff 207838.Jul 3 2019, 10:34 AM

Add comments and CHECK-NOT: __invoke_void

This revision was automatically updated to reflect the committed changes.

Hmm right, every other function has hidden... while you're on a roll, can you remove all hiddens here? :) Thanks!

Friendly ping. Next time, if you don't want to make some changes I suggested, could you please not just ignore it but rather let me know why you don't want that? This is really nothing, so I'm not asking you to upload this in a separate patch or anything so nevermind about this, but I noticed this pattern a few times now, so I'd just mention this. Thanks for the bugfix again.

@aheejin Sorry, did not see that comment.