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

[Thomas Lively via Phabricator via cfe-commits]

tlively accepted this revision.
tlively added a comment.
This revision is now accepted and ready to land.

Thanks for your patience through all the review! Let's get this landed :)



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:19
                          [],
                          "table.get\t$res, $table",
                          "table.get\t$table",
----------------
pmatos wrote:
> tlively wrote:
> > pmatos wrote:
> > > tlively wrote:
> > > > 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.
> > > Not sure I understand why this should be `$i` instead of `$table`. Isn't `$table` represented as an index anyway? A `table32_op` is defined as `Operand<i32>` so I don't quite understand what we gain by changing this.
> > I would still expect to see a register for the result and immediate inputs for the table symbol and the table slot index here.
> Not sure I understood everything properly as the only thing missing was the index in the register based version. I added that now.
Ah, sorry about that. I was confused and was thinking that the table index was an immediate, but of course you're right that it is not.


================
Comment at: llvm/test/CodeGen/WebAssembly/funcref-call.ll:15
+; CHECK-NEXT: local.get 0
+; CHECK-NEXT: table.set __funcref_call_table
+; CHECK-NEXT: local.get 0
----------------
pmatos wrote:
> tlively wrote:
> > Missing a table element index here.
> I am not sure whether that's the case. According to the spec, table.set only gets a table. Two elements are popped from the stack: the index `i32.const 0` and the value `local.get 0`.
Yep, sorry for the confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425