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

[Paulo Matos via Phabricator via llvm-commits]

pmatos marked 7 inline comments as done.
pmatos added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1174
+  } else
+    OS << ", opaque ";
   if (getAlign() != getBaseAlign())
----------------
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


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:132-134
+    // Setting Comdat ensure only one table is left after linking when multiple
+    // modules define the table.
+    Sym->setComdat(true);
----------------
sbc100 wrote:
> tlively wrote:
> > Is this necessary given that the symbol is set to be undefined? Perhaps it would be better to make it defined here and also set comdat so that the linker doesn't need to do anything special to make sure it exists in the final binary. (Or possibly I'm misunderstanding what these settings mean!) @sbc100 wdyt?
> I think don't want this here.  Firstly I think this will mark the symbol as being a comdat group symbol .. which is not the same thing as being part of a comdat group ( which I think is that is indented here).    Secondly, I don't think it makes sense for an undefined symbol to be part of a comdat group.  I think what you want here, if the symbol was defined is `setWeak()`.    Given that is not defined I dont think you even want that.
It seems I was mistaken - I thought that by setting it as Comdat, the linker would merge symbols from different compilation units if they existed. Maybe what I need is `Weak` then.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:59-61
+  // AS 2 : is an integral address space for globals of externref values
+  // AS 3 : is an non-integral address space for funcref values
+  // AS 4 : is an integral address spaces for globals of funcref values
----------------
tlively wrote:
> Why are the address spaces for globals integral? It doesn't make sense to do arithmetic on the "address" of a global afaict. Does using getelementptr to index tables in the global address space require the address space to be integral? If so, that might be a good reason to make the address space integral, but it might also be a good reason to put tables and globals in different address spaces instead. Also, IMO it would be better to use just a single address space for all globals; that way we could support globals with other types like i32, i64, etc. without even more address spaces.
Current set of patches should have addressed your concerns here.


================
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),
----------------
tlively wrote:
> wingo wrote:
> > I think you may need `hasSideEffects = 0` for these annotations to have an effect.
> I would be surprised if this were true!
Why would this be the case? If I remember correctly, I added `mayLoad` and `mayStore` here so that lowering includes a chain. And this works without the need for `hasSideEffects`. Unless you think this is required for other reaons, but `mayLoad` works with it.


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


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:129
+                        TT.isArch64Bit()
+                            ? (hasReferenceTypes(FS)
+                                   ? "e-m:e-p:64:64-i64:64-n32:64-S128-ni:1:3"
----------------
tlively wrote:
> This check should incorporate the CPU as well using `getSubtargetImpl(CPU, FS)->hasReferenceTypes()`.
What exactly do you mean here? Not sure what is the difference here. You mean adding something like : `hasReferenceTypes(FS) || getSubtargetImpl(CPU, FS)->hasReferenceTypes()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425