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

[Paulo Matos via Phabricator via cfe-commits]

pmatos marked 6 inline comments as done.
pmatos added a comment.

Hopefully we are close to landing this. :)



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:19
                          [],
                          "table.get\t$res, $table",
                          "table.get\t$table",
----------------
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.


================
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
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425