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