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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits