[PATCH] D111227: [WebAssembly] Implementation of table.grow/size and ref.null intrinsics

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: .gitignore:63
 .clangd/
+.ccls-cache/
 .cache
----------------
Ignore this for now... local change committed by mistake.


================
Comment at: llvm/include/llvm/IR/Intrinsics.h:162
       AK_AnyPointer,
+      AK_AnyRef,
       AK_MatchType = 7
----------------
This single change caused me to lose countless hours, until I understood that in `IntrinsicsEmitter.cpp`, the order of the `case`s statement for `rAny` depends on the order of this. I added some comments in `IntrinsicsEmitter.cpp` to ensure others don't lose the same amount of time. However, this (the comments) could be a separate patch.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:238
   def llvm_anyvector_ty  : LLVMType<vAny>;
+  def llvm_anyref_ty     : LLVMType<rAny>;
 }
----------------
Maybe this could be a separate patch - adding the new polymorphic type, what do you think?


================
Comment at: llvm/include/llvm/IR/Type.h:80
 
+  ////////// FIXME: WASM EXPERIMENTAL 
+  enum WasmAddressSpace : unsigned {
----------------
I would appreciate some suggestions here. The predicate to recognize reference types, which is required by the polymorphic type `llvm_anyref_ty` is currently defined in WebAssemblyUtils. However, we need it to be in `Type.h`. What can we do here? It feels like pulling all this target dependent stuff into here is just not ideal. Minimally, we could include the WebAssemblyUtils header here and create a wrapper for these predicates in `Type`. But that still involves including a target dependent header here. Any suggestions?


================
Comment at: llvm/lib/CodeGen/ValueTypes.cpp:207
+    // FIXME: unsure this correct but before it was definitely NOT!
+    return PointerType::get(0, 20); // opaque pointer to addrspace(20)
   case MVT::v1i1:
----------------
tlively wrote:
> pmatos wrote:
> > tlively wrote:
> > > Maybe we should go along with the transition to opaque pointers and do `PointerType::get(Context, 20)`? If so, we should probably do the same for `externref`.
> > Tbh, I thought I was using that here when doing `PointerType::get(0, 20)`, but you're right. I need to do `PointerType::get(Context, 20)`. When I do so, Iearn that I need to enable opaque pointers mode through `Context.enableOpaquePointers()`.
> > 
> > Curiously, nobody calls that function at the moment, but I have now done so to understand the consequences for us and what kind of pandora's box I am opening.
> Oh hmm, that doesn't sound like something we want to do. `Context.enableOpaquePointers()` sounds like some sort of global configuration that we don't want to be responsible for here.
> 
> I think the `PointerType::get(0, 20)` is interpreted as passing nullptr as the `Type*` parameter.
I took the opaque pointer stuff a bit too far and got stuck in a few places where LLVM still looks for the pointee type and crashed, which should be fixed but felt like I was diving into a pandora's box.

After discussing with @wingo he suggested using pointers to i8, which works... 
Although I feel the cleaner solution is opaque pointers, I am unsure we should be the first users of opaque pointers in LLVM at the moment. Therefore funcrefs are now pointers to i8 which are cast to the respective type when used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111227