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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 3 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);
 }
----------------
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?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:338
+//def : Pat<(i64 (WebAssemblywrapper tglobaladdr:$addr)),
+//          (GLOBAL_GET_I64 tglobaladdr:$addr)>, Requires<[IsPIC,HasAddr64]>;
 
----------------
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.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83729