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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:331
 def : Pat<(i32 (WebAssemblywrapper tglobaladdr:$addr)),
-          (CONST_I32 tglobaladdr:$addr)>, Requires<[IsNotPIC]>;
+          (CONST_I32 tglobaladdr:$addr)>, Requires<[IsNotPIC,HasAddr32]>;
+def : Pat<(i64 (WebAssemblywrapper tglobaladdr:$addr)),
----------------
Nit: a comma after `,`, the same for lines below


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


================
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:
> 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?


================
Comment at: llvm/test/CodeGen/WebAssembly/pointer64.ll:10
+; Function Attrs: noinline nounwind
+define hidden void @bar(i32 %n) #0 {
+entry:
----------------
Nit: We can remove `hidden` and `#0`, and the attr comments `; Function Attrs: noinline nounwind`. The same for other functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83729