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

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.td:240
 
 // Pseudo valuetype mapped to the current pointer size.
 def iPTR       : ValueType<0, 254>;
----------------
Hmm, I wonder what "current pointer size" means here. The contrast between this and `iPTRAny` makes me think that there should be a way to control the address space of `iPTR` arguments. Maybe it's the address space attached to the call instruction for the intrinsic? If we can find a target-independent way to pass pointers to particular address spaces to intrinsics, that seems like it would be a better solution that putting specific knowledge of WebAssembly address spaces into IR/Type.h. This also might be worth asking about on llvm-dev.


================
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:
----------------
pmatos wrote:
> 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.
Using pointers to i8 seems fine to me. Another option would be using pointers to pointers to opaque structs like we do (IIRC) with externrefs. Or maybe externrefs should also be pointers to i8. It would nice to be consistent between them if funcrefs can't just be function pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111227