[AArch64] Remove implicit landing pads.

[danielkiss]

Summary

PACIASP and PACIA LR, SP are equivalent from PAC point of view while PACIA is not a BTI landing pad.
Direct branch does not need a landing pad. If PACIA is available from v8.3, the unnecessary landing pads could be eliminated.
Another patch will improve the detection of the direct only calls.

[chill]

llvm/lib/Target/AArch64/AArch64BranchTargets.cpp

129

Suggest using an explicit condition, to spare the reader back-inferencing what conditions could set HintNum to 32.

134

Why not create the PACI[AB] from the very start, with a comment why are we doing it?

[chill]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

940 ↗

(On Diff #256025)

Why not create the PACI[AB] from the very start, with a comment why are we doing it?

Sorry, I meant do say, create in the prologue:

• PACIA if we do know the function should/can not be called indirectly and "branch-target-enforcement" attribute is present (if it's absent, it doesn't matter which insn we emit, right?)
• PACIASP otherwise

Then this BTI placrement pass could just check

if (MBBI->getOpcode() == AArch64::PACIASP ||
  MBBI->getOpcode() == AArch64::PACIBSP ||
  MBBI->getOpcode() == AArch64::PACIA ||
   MBBI->getOpcode() == AArch64::PACIB) 
  return false;

knowing that what needs to be done has already been done.

941 ↗

(On Diff #256025)

Could this be refactored in a separate function and shared with the similar code that signs outlined functions?

[danielkiss]

I'm going to refactor this and rebase on my other changes.

[danielkiss]

Simplify the patch, rebased.

[danielkiss]

llvm/lib/Target/AArch64/AArch64BranchTargets.cpp

134

Just replacing them here is a why more simpler code.
I won't change the generated code if BTI is not on.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

941 ↗

(On Diff #256025)

I'm going to create a new patch that removes the code duplication.

[chill]

llvm/lib/Target/AArch64/AArch64BranchTargets.cpp

134

I don't find that simpler or logical at all. We have a function called addBTI and it *removes" a BTI?
The only two things that make sense are:

• create the right instruction from the very start, we have al the necessary information to do so, it makes sense doing the right thing on the spot, instead of patching it later, which in addition to being harder to understand (you have to have knowledge and be aware of multiple parts of the code in order to figure out what's at the beginning of a function and also has time/space overheads, or
• create the non-BTI instruction and the add the BTI property in the pass that adds the BTIs

[danielkiss]

I'll update this after other PAC/BTI patches got merged.

[danielkiss]

see D99417, way different approach is needed to reduce the number of landing pads.

llvm/lib/Target/AArch64/AArch64BranchTargets.cpp

 public:
   static char ID;
   AArch64BranchTargets() : MachineFunctionPass(ID) {}
   void getAnalysisUsage(AnalysisUsage &AU) const override;
   bool runOnMachineFunction(MachineFunction &MF) override;
   StringRef getPassName() const override { return AARCH64_BRANCH_TARGETS_NAME; }
 
 private:
-  void addBTI(MachineBasicBlock &MBB, bool CouldCall, bool CouldJump);
+  bool addBTI(MachineBasicBlock &MBB, bool CouldCall, bool CouldJump);
 };
 } // end anonymous namespace
 
 char AArch64BranchTargets::ID = 0;
 
 INITIALIZE_PASS(AArch64BranchTargets, "aarch64-branch-targets",
                 AARCH64_BRANCH_TARGETS_NAME, false, false)
 
for (MachineBasicBlock &MBB : MF) {
     if (&MBB == &*MF.begin() && (F.hasAddressTaken() || !F.hasLocalLinkage()))
       CouldCall = true;
 
     // If the block itself is address-taken, it could be indirectly branched
     // to, but not called.
     if (MBB.hasAddressTaken() || JumpTableTargets.count(&MBB))
       CouldJump = true;
 
-    if (CouldCall || CouldJump) {
-      addBTI(MBB, CouldCall, CouldJump);
-      MadeChange = true;
-    }
+    MadeChange |= addBTI(MBB, CouldCall, CouldJump);
   }
 
   return MadeChange;
 }
 
-void AArch64BranchTargets::addBTI(MachineBasicBlock &MBB, bool CouldCall,
+bool AArch64BranchTargets::addBTI(MachineBasicBlock &MBB, bool CouldCall,
                                   bool CouldJump) {
   LLVM_DEBUG(dbgs() << "Adding BTI " << (CouldJump ? "j" : "")
                     << (CouldCall ? "c" : "") << " to " << MBB.getName()
                     << "\n");
 
   const AArch64InstrInfo *TII = static_cast<const AArch64InstrInfo *>(
       MBB.getParent()->getSubtarget().getInstrInfo());
 
   unsigned HintNum = 32;
   if (CouldCall)
     HintNum |= 2;
   if (CouldJump)
     HintNum |= 4;
-  assert(HintNum != 32 && "No target kinds!");
 
   auto MBBI = MBB.begin();
 
-  // Skip the meta instuctions, those will be removed anyway.
+  // Skip the meta instructions, those will be removed anyway.
   for (; MBBI != MBB.end() && MBBI->isMetaInstruction(); ++MBBI)
     ;
 
+  if (MBBI == MBB.end())
+    return false;
+
   // PACI[AB]SP are implicitly BTI JC, so no BTI instruction needed there.
-  if (MBBI != MBB.end() && (MBBI->getOpcode() == AArch64::PACIASP ||
-                            MBBI->getOpcode() == AArch64::PACIBSP))
-    return;
+  if (MBBI->getOpcode() == AArch64::PACIASP ||
+      MBBI->getOpcode() == AArch64::PACIBSP) {
+    // Only direct jump could reach this function, therefore the implicit
+    // landing could be removed.
+    if (HintNum == 32) {
+      const AArch64Subtarget &Subtarget =
+          MBB.getParent()->getSubtarget<AArch64Subtarget>();
+
+      if (Subtarget.hasV8_3aOps()) {
+        // PACI[A|B] LR, SP performs the same operation but it is not a landing
+        // pad.
+        BuildMI(MBB, MBBI, MBB.findDebugLoc(MBB.begin()),
+                TII->get(MBBI->getOpcode() == AArch64::PACIASP
+                             ? AArch64::PACIA
+                             : AArch64::PACIB))
+            .addReg(AArch64::LR)
+            .addReg(AArch64::SP);
+        MBB.erase(MBBI);
+        return true;
+      }
+    }
+    return false;
+  }
 
+  if(HintNum == 32)
+    return false;
   BuildMI(MBB, MBB.begin(), MBB.findDebugLoc(MBB.begin()),
           TII->get(AArch64::HINT))
       .addImm(HintNum);
+  return true;
 }

llvm/test/CodeGen/AArch64/branch-target-enforcement-direct-calls.ll

+; RUN: llc -mtriple aarch64--none-eabi -mattr=+bti < %s | FileCheck %s --check-prefixes=CHECK,CHECK8_0
+; RUN: llc -mtriple aarch64--none-eabi -mattr=+bti -mattr=v8.3a < %s | FileCheck %s --check-prefixes=CHECK,CHECK8_3
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-arm-none-eabi"
+
+; When BTI is enabled and Arm v8.3 is avaliable then in case of function that is called
+; only with direct branch the landing pad should be replaced with an equivalent non landing
+; instruction.
+
+define internal void @localfunc() #0 {
+;CHECK:  localfunc:
+;CHECK8_0:          hint    #25
+;CHECK8_3:          pacia x30, sp
+entry:
+  %l1 = alloca i32, align 4
+  ret void
+}
+
+; Global function should have landing pad. Note hint #25 is paciasp.
+;CHECK: bti_enabled:
+;CHECK8_0:        hint    #25
+;CHECK8_3:        paciasp
+define void @bti_enabled() #0  {
+entry:
+  %l1 = alloca i32, align 4
+  tail call void @localfunc()
+  ret void
+}
+
+attributes #0 = { noinline  "branch-target-enforcement"="true"  "sign-return-address"="all" "sign-return-address-key"="a_key" }
 No newline at end of file