This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] 64-bit (function) pointer fixes.
ClosedPublic

Authored by aardappel on Jul 13 2020, 4:35 PM.

Details

Summary

Accounting for the fact that Wasm function indices are 32-bit, but in wasm64 we want uniform 64-bit pointers.
Includes reloc types for 64-bit table indices.

Diff Detail

Event Timeline

aardappel created this revision.Jul 13 2020, 4:35 PM
sbc100 accepted this revision.Jul 13 2020, 4:48 PM
sbc100 added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

This seems unrelated.. and untested?

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
338

Why commented out ? Here and below?

llvm/test/CodeGen/WebAssembly/pointer64.ll
1 ↗(On Diff #277608)

Don't we already have a function pointer test that we can update to test both arches?

This revision is now accepted and ready to land.Jul 13 2020, 4:48 PM

The part with the wrap lgtm!

aardappel marked 3 inline comments as done.Jul 13 2020, 5:26 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

This is something I ran into while testing this code. It not exiting caused problems further down the line in the wasm backend which it shouldn't.. this is strictly a bug in non-wasm code, but didn't want to touch that. Not sure how you'd even test this. Do you want this in a review by itself?

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
338

these cause tablegen/isel ambiguity errors, I thought maybe someone would have an idea why. They're not needed for the current functionality so I can remove them.

llvm/test/CodeGen/WebAssembly/pointer64.ll
1 ↗(On Diff #277608)

This tests things very specific to wasm64 (e.g. the truncation), so I prefer a test that focuses just on what's new.

sbc100 added inline comments.Jul 13 2020, 5:41 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

Ok, I guess I don't really understand the comment. Is diagnose supposed to call exit? Should we have a TODO here if so? I'm not sure what you mean by handler either.

llvm/test/CodeGen/WebAssembly/pointer64.ll
1 ↗(On Diff #277608)

Ok, you about we give it a more specific name then. test_call_indirect_wasm64.ll?

tlively added inline comments.Jul 13 2020, 5:45 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

If this is a bug in non-wasm code it should probably be fixed properly in a separate patch. You can throw it up and add one of us plus anyone who touched the relevant code recently as reviewers.

aheejin added inline comments.Jul 13 2020, 11:54 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
331–333

Nit: a comma after ,, the same for lines below

338

The i32 version of this pattern seems to be used for PIC tests, such as call-pic.ll or load-store-pic.ll. I guess we need this 64 bit pattern if we compile those files with wasm64 triple. Not sure what the ambiguity error is, but maybe we can fix it here or address it in a separate patch.

llvm/test/CodeGen/WebAssembly/pointer64.ll
1 ↗(On Diff #277608)

Do we need -O2 here?

10 ↗(On Diff #277608)

Nit: We can remove hidden and #0, and the attr comments ; Function Attrs: noinline nounwind. The same for other functions.

aardappel marked 5 inline comments as done.Jul 16 2020, 12:26 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

I don't even know how to fix this in common code, or if that even would be desirable. The diagnostic being raised is of type Error, which, if there is no handler, goes into common code and calls exit (such that following code can rely on the program not continuing, since code further downstream doesn't support this feature). However, if there is a handler, it goes into a web of handler-handling, and really should be calling exit somewhere, but doesn't. I don't know what the fix is there, and I am ware of adding an exit in common code that might break god knows what for someone else.

Anyway, I can take it out of this patch and into another one.

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
338

I will remove them for now, we can address this later.

llvm/test/CodeGen/WebAssembly/pointer64.ll
1 ↗(On Diff #277608)

it doesn't just test call_indirect, but I will make the name more specific.

without -O2 it emits a bunch of redundant gets/sets that I thought would clutter up the test.

Code review fixes

aheejin added inline comments.Jul 16 2020, 12:42 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

Yeah I also think addressing this in a separate patch is better. Also it would be helpful to know which test cases triggers a bug you tried to fix by this.

aheejin accepted this revision.Jul 16 2020, 12:42 PM
aardappel marked an inline comment as done.Jul 16 2020, 1:47 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
689

it was a gcc torture test using an unsupported builtin which then (because it doesn't exit) causes further errors. It really isn't that big a deal.

This revision was automatically updated to reflect the committed changes.