pmatos 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"
----------------
tlively wrote:
> 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.
Interesting - had not come accross it. Bleeding edge does not seem to include 
reference-types. What's the reason for this? 


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

Reply via email to