tlively accepted this revision.
tlively added a comment.
This revision is now accepted and ready to land.
Thanks!
================
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:
> > pmatos wrote:
> > > 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?
> > We don't have a well-defined process for adding features to bleeding-edge,
> > but I think typically they're added once they're mostly stable and usable
> > in the tools.
> @tlively I have now unconditionally updated the layout string. No failures.
> How does this look like now?
Looks good, thanks!
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
