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

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

It would be good to add dedicated tests for table.get and table.set as well.



================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1174
+  } else
+    OS << ", opaque ";
   if (getAlign() != getBaseAlign())
----------------
pmatos wrote:
> tlively wrote:
> > Is there a test that demonstrates this printing? Also, I'm not sure specifically calling out zero-sized operands in the text dump is that useful. Can zero-sized operands still be given an alignment? If so, it might even be good to continue printing the alignment for them.
> The reason I changed this here is due to the call of getSize() crashing for zeroSized arguments. I have not added test for the printing. I don't think zero-sized operands can still be given an alignment. We are actually setting the alignment to
Looks like this comment was cut off! I wonder if it would be better to skip printing `opaque` for now since we have no test of it/ Also, if another target comes along with a zero-sized type, I don't know whether "opaque" will necessarily describe it.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:558-559
 
+  // If this is a funcref call, to avoid hidden GC roots, we need to clear the
+  // table slot with ref.null upon call_indirect return.
+  if (IsIndirect && IsFuncrefCall) {
----------------
Can you add the .wat of the code being generated here to the comment?


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


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-undef.ll:13-14
+; CHECK-LABEL: return_extern_undef:
+; CHECK-NEXT: functype       return_extern_undef () -> ()
+; CHECK-NEXT: end_function
+
----------------
tlively wrote:
> pmatos wrote:
> > tlively wrote:
> > > I would expect this to declare an externref local and return it uninitialized or to return a ref.null. It would be good to at least add a TODO comment for fixing this.
> > Returning a `ref.null` or an uninitialized externref would make more sense if the return type would be `%externref`. However, in this case this is an `%extern` value, which really is an opaque type and should never really happen.
> Ah, that makes sense.
It would be good to add this explanation as a comment in the test file.


================
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
----------------
Missing a table element index here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425