[BOLT] Check for !isTailCall in isUnconditionalBranch

[yota9]

Summary

Add !isTailCall in isUnconditionalBranch check in order to sync the x86
and aarch64 and fix the fixDoubleJumps pass on aarch64.

Vladislav Khmelevsky,
Advanced Software Technology Lab, Huawei

[yota9]

Add test description

[rafauler]

Thanks for fixing! I think we should probably modify MCPlusBuilder.h instead. In that way we make the AArch64 backend behave in the same way as the X86 one.

bolt/lib/Passes/BinaryPasses.cpp

700 ↗

(On Diff #419827)

This is out of sync with X86. In X86, isUnconditionalBranch only returns true if it is also not a tail call:

return Analysis->isBranch(Inst) && !isTailCall(Inst);

For consistency, AArch64MCPlusBuilder should probably do the same at MCPlusBuilder.h:356.

[yota9]

@rafauler Thanks for your suggestion, I've missed that moment, I think you're right

[Amir]

bolt/test/AArch64/ext-double-jump.s

6

We already have -nostartfiles and -nostdlib in %cflags (bolt/test/AArch64/lit.local.cfg). Do we still need -nostartfiles -nodefaultlibs here?

[yota9]

bolt/test/AArch64/ext-double-jump.s

6

Basically no, but from the stand-alone test point I would like them to be in such tests, where we don't need them by design

[maksfb]

bolt/test/AArch64/ext-double-jump.s

11

nit: could you add size directives for functions in the test?

[yota9]

bolt/test/AArch64/ext-double-jump.s

11

Sure, will do. Does it affect something? I did it before, but usually BOLT works just fine without it, just curious

[maksfb]

Actually, I'm not convinced it's a great idea to check for the tail call here.

bolt/test/AArch64/ext-double-jump.s

11

That's the proper way to mark functions in code. Ideally, you also need FDEs. Without this info, BOLT has to rely on heuristics to detect function boundaries.

[yota9]

bolt/test/AArch64/ext-double-jump.s

11

Yes, I remember these heuristics, I just thought you've faced with some problems during cross-target tests enabling :)

[maksfb]

bolt/test/AArch64/ext-double-jump.s

11

Ah no, it's just a nit.

bolt/include/bolt/Core/MCPlusBuilder.h

public:
 
   /// Returns true if Inst is a condtional move instruction
   virtual bool isConditionalMove(const MCInst &Inst) const {
     llvm_unreachable("not implemented");
     return false;
   }
 
   virtual bool isUnconditionalBranch(const MCInst &Inst) const {
-    return Analysis->isUnconditionalBranch(Inst);
+    return Analysis->isUnconditionalBranch(Inst) && !isTailCall(Inst);
   }
 
   virtual bool isIndirectBranch(const MCInst &Inst) const {
     return Analysis->isIndirectBranch(Inst);
   }
 
   /// Returns true if the instruction is memory indirect call or jump
   virtual bool isBranchOnMem(const MCInst &Inst) const {

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

public:
   X86MCPlusBuilder(const MCInstrAnalysis *Analysis, const MCInstrInfo *Info,
                    const MCRegisterInfo *RegInfo)
       : MCPlusBuilder(Analysis, Info, RegInfo) {}
 
   bool isBranch(const MCInst &Inst) const override {
     return Analysis->isBranch(Inst) && !isTailCall(Inst);
   }
 
-  bool isUnconditionalBranch(const MCInst &Inst) const override {
-    return Analysis->isUnconditionalBranch(Inst) && !isTailCall(Inst);
-  }
-
   bool isNoop(const MCInst &Inst) const override {
     return X86::isNOP(Inst.getOpcode());
   }
 
   unsigned getCondCode(const MCInst &Inst) const override {
     unsigned Opcode = Inst.getOpcode();
     if (X86::isJCC(Opcode))
       return Inst.getOperand(Info->get(Opcode).NumOperands - 1).getImm();

bolt/test/AArch64/ext-double-jump.s

+# This test checks that remove double jumps pass works properly with
+# non-local branches.
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags -nostartfiles -nodefaultlibs %t.o -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.bolt -peepholes=double-jumps
+
+  .text
+  .align 4
+  .global dummy1
+  .type dummy1, %function
+dummy1:
+  mov x2, x0
+  ret
+  .size dummy1, .-dummy1
+
+  .global dummy2
+  .type dummy2, %function
+dummy2:
+  mov x1, x0
+  ret
+  .size dummy2, .-dummy2
+
+  .global _start
+  .type _start, %function
+_start:
+  cbz  x10, 1f
+  b dummy1
+1:
+  b dummy2
+  .size _start, .-_start