[BOLT][AArch64] Treat br x30 as a return

[maksfb]

On AArch64, br x30 instruction is equivalent to ret. Treat it as such while analysing the control flow.

Without the special case, br x30 is considered an indirect branch with an unknown control flow making a control flow analysis more complicated than necessary.

We can also replace br x30 with ret. I'd rather do it in a separate pass to allow tools built on top of BOLT to have access to the original assembly.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Maksim Panchenko (maksfb)

Changes

On AArch64, br x30 instruction is equivalent to ret. Treat it as such while analysing the control flow.

Without the special case, br x30 is considered an indirect branch with an unknown control flow making a control flow analysis more complicated than necessary.

We can also replace br x30 with ret. I'd rather do it in a separate pass to allow tools built on top of BOLT to have access to the original assembly.

---

Full diff: https://github.com/llvm/llvm-project/pull/159458.diff

2 Files Affected:

• (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+21)
• (added) bolt/test/AArch64/br-x30.s (+16)

diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index f972646aa12ea..b8da5d854b836 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -152,6 +152,27 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     return isLoadFromStack(Inst);
   };
 
+  bool isReturn(const MCInst &Inst) const override {
+    // BR X30 is equivalent to RET
+    if (Inst.getOpcode() == AArch64::BR &&
+        Inst.getOperand(0).getReg() == AArch64::LR)
+      return true;
+
+    return Analysis->isReturn(Inst);
+  }
+
+  bool isBranch(const MCInst &Inst) const override {
+    if (isReturn(Inst))
+      return false;
+    return Analysis->isBranch(Inst);
+  }
+
+  bool isIndirectBranch(const MCInst &Inst) const override {
+    if (isReturn(Inst))
+      return false;
+    return Analysis->isIndirectBranch(Inst);
+  }
+
   void createCall(MCInst &Inst, const MCSymbol *Target,
                   MCContext *Ctx) override {
     createDirectCall(Inst, Target, Ctx, false);
diff --git a/bolt/test/AArch64/br-x30.s b/bolt/test/AArch64/br-x30.s
new file mode 100644
index 0000000000000..711e8a8faef84
--- /dev/null
+++ b/bolt/test/AArch64/br-x30.s
@@ -0,0 +1,16 @@
+## Test that "br x30" is treated as a return instruction and not as an indirect
+## branch.
+
+# RUN: %clang %cflags %s -o %t.exe -Wl,-q -Wl,--entry=foo
+# RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg 2>&1 | FileCheck %s
+
+# CHECK: BB Count : 2
+# CHECK-NOT: UNKNOWN CONTROL FLOW
+
+  .text
+  .global foo
+  .type foo, %function
+foo:
+  br x30
+  nop
+  .size foo, .-foo

[kbeyls]

Hmmm....

There are differences between the semantics of RET x30 and BR x30.
For example, when the Armv9.3 Guarded Control Stack feature is enabled, the BR is not considered a return and does not alter the guarded control stack, while the RET is considered a return and does update the guarded control stack state. The pseudo code for the RET instruction contains the following pseudo-code, while the pseudo code for BR does not:

if IsFeatureImplemented(FEAT_GCS) && GCSPCREnabled(PSTATE.EL) then
    target = LoadCheckGCSRecord(target, GCSInstType_PRET);
    SetCurrentGCSPointer(GetCurrentGCSPointer() + 8);

Also for BTI (which is already widely deployed), there is a difference.
The BR pseudo code related to BTI is:

// Value in BTypeNext will be used to set PSTATE.BTYPE
if InGuardedPage then
  if n == 16 || n == 17 then
    BTypeNext = '01';
  else
    BTypeNext = '11';
else
    BTypeNext = '01';

whereas the RET pseudo code relate to BTI is:

// Value in BTypeNext will be used to set PSTATE.BTYPE
BTypeNext = '00';

As GCS (guarded control stack) gets deployed more widely, I would assume that most code that uses BR x30 when it really should use RET will need to be updated.
Do you see BR x30 frequently enough in code in the wild that it makes a meaningful difference to recognize it as a return in BOLT?

[paschalis-mpeis]

RET does BR x30 by default, but a BR x30 is not necessarily a return.

In the PCS, x30 is the LR, but it can still be used as a GP in custom assembly.

Do you see BR x30 frequently enough in code in the wild that it makes a meaningful difference to recognize it as a return in BOLT?

It'll be interesting to see where such cases comes from, and as Kristof says, how often they occur.

At minimum, we could emit some warnings to increase visibility and help improve the input sources (where is needed), or guard this simplification behind an experimental flag?

[maksfb]

Thanks for the details on the differences between the two instructions.

For the part where we decide how to treat br x30 while building the CFG, I believe we can add a sanity check for x30/LR usage in the containing function. If custom assembly code uses x30 in a non-standard manner and then issues either br x30 or ret, then the control flow analysis could be wrong.

For instruction conversion, which I don't yet plan to implement, it makes sense to perform such only under an option and issue a warning.

As to how frequent br x30 occurs, I don't have the data, I only know that the code comes from assembly. @yozhu, could you please share which libraries have it?

[yozhu]

br x30 is used quite often in this assembly file: https://github.com/mozilla/mozjpeg/blob/master/simd/arm/aarch64/jsimd_neon.S