Page MenuHomePhabricator

[AArch64][SVE] Fix CFA calculation in presence of SVE objects.
ClosedPublic

Authored by sdesmalen on Jul 17 2020, 9:40 AM.

Details

Summary

The CFA is calculated as (SP/FP + offset), but when there are
SVE objects on the stack the SP offset is partly scalable and
should instead be expressed as the DWARF expression:

SP + offset + scalable_offset * VG

where VG is the Vector Granule register, containing the
number of 64bits 'granules' in a scalable vector.

Diff Detail

Event Timeline

sdesmalen created this revision.Jul 17 2020, 9:40 AM
Herald added a project: Restricted Project. · View Herald Transcript
sdesmalen updated this revision to Diff 279483.Jul 21 2020, 4:47 AM

Use SmallString instead of SmallVector<uint8_t>.

sdesmalen updated this revision to Diff 280501.Jul 24 2020, 9:39 AM

rebased patch.

Gentle ping because D84044 can't land without this patch :)

I'm not really familiar enough with DWARF expressions to give this a thorough review, but I don't see anything obviously wrong.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
449

It seems unfortunate that we have to emit a stream of raw bytes here. It would be nice if we could at least emit a comment explaining what the bytes are supposed to mean; maybe we could stick a string in MCCFIInstruction?

sdesmalen updated this revision to Diff 282451.Aug 2 2020, 8:04 AM

Added comment to .cfi_escape to describe CFA DWARF expression.

Can't say I know a ton about CFI/debug_frame, unfortunately. The DWARF expression does match up with the comment - so if the intent is good, the implementation looks good to me, FWIW. (comment might be a bit clearer though, as noted)

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
403–404

Might be more accurate to describe this as:

//   Expr + NumBytes + NumVGScaledBytes * AArch64::VG

Since it includes the addition of the existing element in Expr.

efriedma accepted this revision.Aug 3 2020, 1:37 PM

LGTM with one minor comment

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
440

Why "x31" and not "sp"?

This revision is now accepted and ready to land.Aug 3 2020, 1:37 PM
This revision was automatically updated to reflect the committed changes.
sdesmalen marked 2 inline comments as done.

Thanks for the reviews!

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
403–404

Good point, thanks!

440

You're right, x31 seems wrong. I've changed it to sp before committing.

rupprecht added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
443–445

I'm seeing the following warning here:

[723/2438] Building CXX object lib/Target/AArch64/CMakeFiles/LLVMAArch64CodeGen.dir/AArch64FrameLowering.cpp.o
/home/rupprecht/src/llvm-project/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:445:37: warning: implicit conversion from 'int' to 'const char' changes value from 143 to -113 [-Wconstant-conversion]
  Expr.push_back(dwarf::DW_OP_breg0 + /*SP*/ 31);
       ~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
1 warning generated.

Do you see it as well? Is there a way to silence it?

sdesmalen added inline comments.Aug 4 2020, 9:57 AM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
443–445

Sorry, I don't see this warning. What compiler are you using?

rupprecht added inline comments.Aug 4 2020, 10:08 AM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
443–445

A recent-ish bootstrapped clang from June 7:

clang++ --version => clang version 11.0.0 (https://github.com/llvm/llvm-project.git ec04ce4623522a326672f03fecb41812386a8c0e)

I'll see if the issue goes away when rolling up to a newer clang

rupprecht added inline comments.Aug 4 2020, 10:50 AM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
443–445

No, still present:

$ clang++ --version
clang version 12.0.0 (https://github.com/llvm/llvm-project.git b7cfa6ca92830b3c331cb44706bb279996663439)

... and, looks like @dblaikie just fixed this in e31cfc4cd3e393300002e9c519787c96e3b67bab.

dblaikie added inline comments.Aug 4 2020, 11:06 AM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
443–445

Yep yep - sorry, was going to follow up to mention that after I committed it, but got a power outage & took me a bit to switch over to hotspotting.