[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–701

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/lib/Passes/BinaryPasses.cpp

auto checkAndPatch = [&](BinaryBasicBlock *Pred, BinaryBasicBlock *Succ,
           assert(Function.getBasicBlockAfter(Pred, false) != Succ &&
                  "Don't add an explicit jump to a fallthrough block.");
           Pred->addBranchInstruction(Succ);
         }
       } else {
         // Succ will be null in the tail call case.  In this case we
         // need to explicitly add a tail call instruction.
         MCInst *Branch = Pred->getLastNonPseudoInstr();
-        if (Branch && MIB->isUnconditionalBranch(*Branch)) {
-          assert(MIB->getTargetSymbol(*Branch) == BB.getLabel());
+        if (Branch && MIB->isUnconditionalBranch(*Branch) &&
+            MIB->getTargetSymbol(*Branch) == BB.getLabel()) {
           Pred->removeSuccessor(&BB);
           Pred->eraseInstruction(Pred->findInstruction(Branch));
           Pred->addTailCallInstruction(SuccSym);
         } else {
           return false;
         }
       }
 

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

+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown \
+# RUN:   %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
+
+  .global dummy2
+  .type dummy2, %function
+dummy2:
+  mov x1, x0
+  ret
+
+  .global _start
+  .type _start, %function
+_start:
+  cbz  x10, 1f
+  b dummy1
+1:
+  b dummy2