[PATCH] D83729: [WebAssembly] 64-bit (function) pointer fixes.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 5 inline comments as done.
aardappel added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:689
+  // diagnose() does not call exit() if there's a handler.
+  exit(1);
 }
----------------
tlively wrote:
> sbc100 wrote:
> > aardappel wrote:
> > > sbc100 wrote:
> > > > This seems unrelated..  and untested?
> > > 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?
> > 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.
> 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.
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:338
+//def : Pat<(i64 (WebAssemblywrapper tglobaladdr:$addr)),
+//          (GLOBAL_GET_I64 tglobaladdr:$addr)>, Requires<[IsPIC,HasAddr64]>;
 
----------------
aheejin wrote:
> aardappel wrote:
> > sbc100 wrote:
> > > Why commented out ? Here and below?
> > 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.
> 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.
I will remove them for now, we can address this later.


================
Comment at: llvm/test/CodeGen/WebAssembly/pointer64.ll:1
+; RUN: llc < %s -asm-verbose=false -O2 | FileCheck %s
+; RUN: llc < %s -asm-verbose=false -O2 --filetype=obj | obj2yaml | FileCheck --check-prefix=YAML %s
----------------
aheejin wrote:
> sbc100 wrote:
> > aardappel wrote:
> > > sbc100 wrote:
> > > > Don't we already have a function pointer test that we can update to test both arches?  
> > > This tests things very specific to wasm64 (e.g. the truncation), so I prefer a test that focuses just on what's new.
> > Ok, you about we give it a more specific name then.  `test_call_indirect_wasm64.ll`?
> Do we need `-O2`  here?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83729/new/

https://reviews.llvm.org/D83729