This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Added test for inline assembly roundtrip.
ClosedPublic

Authored by aardappel on Oct 4 2018, 4:34 PM.

Details

Summary

Due to previous work to make WebAssembly MC by default stack-only
inline assembly now "just works" (previously it didn't since it had
no way to know types of registers), so no further work required.

So far we only have tests (in inline-asm.ll) which test with
non-existing instructions, so this adds a test that roundtrips
both the inline assembly and its surrounding code thru the assembler.

Diff Detail

Event Timeline

aardappel created this revision.Oct 4 2018, 4:34 PM
sbc100 added a comment.Oct 4 2018, 5:06 PM

Sweet!

I kind of prefer the test to be as minimal as possible even if derived from c source, but perhaps thats overly pedantic.

test/CodeGen/WebAssembly/inline-asm-roundtrip.ll
21

drop this line

23

Drop the "-wasm": here and in the command line, like the other tests in this dir

38

Hmm.. is main compiled by clang as hidden? In any case, you can drop that for the test

54

Can you trip all this attribute stuff to keep the test nice and small?

aheejin added inline comments.Oct 4 2018, 5:35 PM
test/CodeGen/WebAssembly/inline-asm-roundtrip.ll
38

Maybe local_unnamed_addr too

54

I usually delete all the attributes unless they are necessary for the current test, but maybe it's because I'm pedantic too

aheejin added inline comments.Oct 4 2018, 5:38 PM
test/CodeGen/WebAssembly/inline-asm-roundtrip.ll
54

Oh, these are not attributes but debug info.. but anyway, maybe we need neither of them here

aardappel updated this revision to Diff 168719.Oct 8 2018, 3:43 PM
aardappel marked 7 inline comments as done.

Addressing code review comments: simplified test.

Simplified test..

If we are using {{$}} on every line, we can probably replace that with FileCheck's [[ https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-match-full-lines | --match-full-lines option ]].

test/CodeGen/WebAssembly/inline-asm-roundtrip.ll
17

Indent these five lines to match the top (?

aardappel updated this revision to Diff 168727.Oct 8 2018, 4:08 PM

More code review fixes.

aardappel marked an inline comment as done.Oct 8 2018, 4:10 PM
aardappel added inline comments.
test/CodeGen/WebAssembly/inline-asm-roundtrip.ll
17

I had originally indented them such that the instructions line up, since the first doesn't have a \t. But that's probably unconventional :)

aheejin added inline comments.Oct 8 2018, 4:36 PM
test/CodeGen/WebAssembly/inline-asm-roundtrip.ll
34

Here the compiler happens to place src and dst in the same local, but I guess that can't be guaranteed. How about changing them to variables, like

; CHECK-NEXT:  i32.const  1
; CHECK-NEXT:  set_local  [[SRC:[0-9]+]]
; CHECK-NEXT:  i32.const  2
; CHECK-NEXT:  get_local  [[SRC]]
; CHECK-NEXT:  i32.add
; CHECK-NEXT:  set_local  [[DST:[0-9]+]]
; CHECK-NEXT:  get_local  [[DST]]
...
aardappel updated this revision to Diff 170528.Oct 22 2018, 5:05 PM
aardappel marked an inline comment as done.

Replaced hard-coded local indices by variables.

aardappel marked an inline comment as done.Oct 22 2018, 5:05 PM
aheejin accepted this revision.Oct 22 2018, 5:06 PM

Thanks!

This revision is now accepted and ready to land.Oct 22 2018, 5:06 PM
This revision was automatically updated to reflect the committed changes.