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