Page MenuHomePhabricator

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

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



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.


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)


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


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!


Good point, thanks!


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

rupprecht added inline comments.

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

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

rupprecht added inline comments.Aug 4 2020, 10:08 AM

A recent-ish bootstrapped clang from June 7:

clang++ --version => clang version 11.0.0 ( 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

No, still present:

$ clang++ --version
clang version 12.0.0 ( b7cfa6ca92830b3c331cb44706bb279996663439)

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

dblaikie added inline comments.Aug 4 2020, 11:06 AM

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.