[PATCH] D95425: Implementation of global.get/set for reftypes in LLVM IR

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:15
 multiclass TABLE<WebAssemblyRegClass rt> {
+  let mayLoad = 1 in
   defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table),
----------------
wingo wrote:
> I think you may need `hasSideEffects = 0` for these annotations to have an effect.
I would be surprised if this were true!


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:19
                          [],
                          "table.get\t$res, $table",
                          "table.get\t$table",
----------------
wingo wrote:
> Not something for this patch, but this is certainly bogus: surely we mean `table.get\t$table, $i` and we have an i32 index argument?
Agree about the i32 index argument, but it is correct to have the result as part of the string for the register-based output format.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-globalget.ll:4
+%extern = type opaque
+%externref = type %extern addrspace(1)* ;; addrspace 1 is nonintegral
+
----------------
wingo wrote:
> tlively wrote:
> > wingo wrote:
> > > A design document for how this works would really be helpful :)  So LLVM is going to consider that any pointer to address space 1 has MVT exterrnref at lowering-time?  It's interesting to go down this route rather than adding a new `llvm::Type`.
> > We essentially consider changes to LLVM IR to be out of scope to the extent possible. If we only touch code in the WebAssembly backend and maybe fix a few bugs in target independent code, then we are comfortable signing off on patches within the team, but if we do anything non-trivial in target independent code, we need sign off from the wider LLVM community. Changes to LLVM IR would require a full-blown RFC process and I expect they would not be accepted.
> I guess my open question is really as regards the C mapping -- would the idea be for the front-end to also specially recognize pointers in address space 1 ?  If there were a first-class IR type for these values, as is the case for SVE, you'd have a straightforward answer.  More discussion in https://docs.google.com/document/d/1aVX0tQChxA2Tlno2KmEUjdruLoNgpwzV5Kv335HvNmI/edit#heading=h.u50o9qhzzjy6.
I forget if we had or resolved this conversation elsewhere already, but I would expect clang to emit pointers in the correct address spaces, yes. The SVE folks had to go though multiple years of effort to add scalable vectors to LLVM IR because there was no existing way to model their semantics in IR such that they could implement all the vectorization optimizations they wanted. We are lucky that we can get away with using address space pointers directly without having to add new types to the IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425