tlively added inline comments.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:130
TT.isArch64Bit()
- ? "e-m:e-p:64:64-i64:64-n32:64-S128-ni:1"
- : "e-m:e-p:32:32-i64:64-n32:64-S128-ni:1",
+ ? (hasReferenceTypes(FS)
+ ?
"e-m:e-p:64:64-i64:64-n32:64-S128-ni:1:10:20"
----------------
pmatos wrote:
> tlively wrote:
> > `hasReferenceTypes` should also be taking the CPU into account, not just
> > the feature string. Normally this would be done via `getSubtargetImpl`, but
> > I guess we probably can't call that this early in the construction of the
> > `WebAssemblyTargetMachine`. Would anything break if we just unconditionally
> > added the reference types address spaces to the data layout string?
> Regarding this change, I don't quite understand why referencetypes should
> take the CPU into account. Are there CPU variants for the wasm backend? I
> haven't touched the conditional because that would mean touching the several
> tests that don't enable reference types and use the old data layout string.
> However, I would think that if that's the path we want to follow here, we
> could do it and change all wasm tests to use the layout string with reference
> types.
>
Yes, there are CPU variants defined here:
https://github.com/llvm/llvm-project/blob/7ac0442fe59dbe0f9127e79e8786a7dd6345c537/llvm/lib/Target/WebAssembly/WebAssembly.td#L89-L100.
Note that the CPU may enable reference types even if the feature string does
not. If it doesn't break anything, then unconditionally updating the layout
string sounds like the best option.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104797/new/
https://reviews.llvm.org/D104797
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
