[llvm] [StackFrameLayoutAnalysis] Use target-specific hook for SP offsets (PR #100386)

Hari Limaye via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 24 11:42:09 PDT 2024


hazzlim wrote:

> > @ilovepi Thanks for reviewing. I was also looking at using the getFrameIndexReference() API directly - but we were keen to have all offsets from a single point of reference. Using the DWARF logic to get CFA relative offsets sounds promising, but maybe this way is simpler for now.
> > > Somehow the YAML tests are still passing, though, which I find surprising. Likely that means we don't have enough coverage. I can follow up on that, and also additional RISC-V tests w/ scalable vectors.
> > 
> > 
> > That doesn’t completely surprise me, if these are non AArch64 tests - this shouldn’t change the YAML output for non AArch64 targets. And should only add the “ScalableOffset” field to the YAML for AArch64 when the value is non-zero.
> > Are you able to point me to the tests you’re referencing? Thanks
> 
> https://github.com/llvm/llvm-project/blob/main/clang/test/Frontend/stack-layout-remark.c, but I can see why those aren't affected, since it `REQUIRES: x86-registered-target`. It may not be too important. The point of that test is to make sure that the YAML output is functional. While your patch modifies the output for Offset, I don't think its impacted any functionallity. Still, I'll probably add a Aarch64 test too, so we have some coverage on Scalable Vectors.

Thanks for pointing me to this test - I can confirm this passes for me locally on an x86 build with this patch applied. To confirm, I don't think my patch modifies the output for Offset unless the target is AArch64 and the function contains Scalable Vector (SVE) stack objects?

That would be great to add an AArch64 test, thank you - agreed it would be ideal to have some Scalable Vector coverage there.

https://github.com/llvm/llvm-project/pull/100386


More information about the llvm-commits mailing list