This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Save FP for leaf functions when disabling frame pointer elimination
ClosedPublic

Authored by MaskRay on Dec 7 2019, 11:10 AM.

Details

Summary

The change allows clang -mno-omit-leaf-frame-pointer to disable frame
pointer elimination. This behavior matches X86 and Mips, and also GCC
AArch64.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 7 2019, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2019, 11:10 AM
MaskRay updated this revision to Diff 232717.Dec 7 2019, 11:48 AM

Fix machine-outliner-regsave.mir. Touch %lr to fix the following -verify-machineinstrs issue

*** Bad machine code: Using an undefined physical register ***                                                                                        
- function:    bar                                       
- basic block: %bb.0  (0x1325e00)                                                                                  
- instruction: early-clobber $sp = STRXpre $lr, $sp(tied-def 0), -16                                                                                  
- operand 1:   $lr
kamleshbhalui added inline comments.Dec 7 2019, 7:57 PM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
247

MFI.hasCalls() was returning here false, Even for function who has call inside. May be because it is called earlier,before generating Full frame Info . It needs to be removed from here. I could that it is done.

efriedma accepted this revision.Dec 13 2019, 11:41 AM

LGTM

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

Isn't TargetOptions::DisableFramePointerElim itself calling hasCalls()? I guess we conservatively return true anyway if the frame info isn't computed yet, though, so probably doesn't matter.

This revision is now accepted and ready to land.Dec 13 2019, 11:41 AM
MaskRay marked 3 inline comments as done.Dec 13 2019, 1:30 PM

Ping:)

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

TargetOptions::DisableFramePointerElim doesn't call hasCalls(). Deleting hasCalls() will make -frame-pointer=[non-leaf|all] different.

MaskRay updated this revision to Diff 233867.Dec 13 2019, 1:31 PM
MaskRay marked an inline comment as done.

Update aarch64-fix-cortex-a53-835769.ll which is sensitive

efriedma added inline comments.Dec 13 2019, 2:25 PM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
247
MaskRay marked an inline comment as done.Dec 13 2019, 3:12 PM
MaskRay added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
247

Thanks for the pointer. It looks there are other cleanup opportunities in TargetOptions::DisableFramePointerElim.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 14 2019, 5:19 AM

This breaks tests on the "expensive tests" bot: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-debian/builds/514

Please take a look!

This breaks tests on the "expensive tests" bot: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-debian/builds/514

Please take a look!

Looking. Where can I find the cmake configuration?

Mine (-DLLVM_ENABLE_ASSERTIONS=On -DLLVM_ENABLE_EXPENSIVE_CHECKS=On) does not build for other reasons:

.../llvm/include/llvm/ADT/PointerUnion.h:93:26: error: constexpr variable 'NumLowBitsAvailable' must be initialized by
 a constant expression                                                                                                                                
    static constexpr int NumLowBitsAvailable = lowBitsAvailable<PTs...>();

This breaks tests on the "expensive tests" bot: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-debian/builds/514

Please take a look!

Looking. Where can I find the cmake configuration?

Mine (-DLLVM_ENABLE_ASSERTIONS=On -DLLVM_ENABLE_EXPENSIVE_CHECKS=On) does not build for other reasons:

.../llvm/include/llvm/ADT/PointerUnion.h:93:26: error: constexpr variable 'NumLowBitsAvailable' must be initialized by
 a constant expression                                                                                                                                
    static constexpr int NumLowBitsAvailable = lowBitsAvailable<PTs...>();

I find the configuration in https://github.com/llvm/llvm-zorg/blob/master/buildbot/osuosl/master/config/builders.py and reproduce with cmake -GNinja -Hllvm -BExpensive -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_EXPENSIVE_CHECKS=On -DLLVM_ENABLE_WERROR=Off -DCMAKE_CXX_FLAGS=-U_GLIBCXX_DEBUG

Fixed in ccc453eb57b91a4e64ecfd7a9ee8d9415345c6b6