[llvm] [BOLT][AArch64] Expand cmpbr when reversing would overflow (PR #202998)

[Rafael Auler via llvm-commits]

================
@@ -2171,38 +2190,75 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     }
   }
 
-  bool isReversibleBranch(const MCInst &Inst) const override {
+  bool isReversibleBranch(const MCInst &Inst,
+                          bool MustPreserveFlags = true) const override {
     if (isCompAndBranch(Inst)) {
       unsigned InvertedOpcode = getInvertedBranchOpcode(Inst.getOpcode());
-      if (needsImmDec(InvertedOpcode) && Inst.getOperand(1).getImm() == 0)
+      if (needsImmDec(InvertedOpcode) && Inst.getOperand(1).getImm() == 0 &&
+          MustPreserveFlags)
         return false;
-      if (needsImmInc(InvertedOpcode) && Inst.getOperand(1).getImm() == 63)
+      if (needsImmInc(InvertedOpcode) && Inst.getOperand(1).getImm() == 63 &&
+          MustPreserveFlags)
         return false;
     }
     return MCPlusBuilder::isReversibleBranch(Inst);
   }
 
-  void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
-                              MCContext *Ctx) const override {
-    if (!isReversibleBranch(Inst)) {
-      errs() << "BOLT-ERROR: Cannot reverse branch " << Inst << "\n";
-      exit(1);
-    }
+  void reverseBranchCondition(BinaryBasicBlock *Parent, MCInst &Inst,
+                              const MCSymbol *TBB, MCContext *Ctx,
+                              bool MustPreserveFlags = true) const override {
+    assert(isReversibleBranch(Inst, MustPreserveFlags) &&
----------------
rafaelauler wrote:

This function requires Parent to be non-null and invalidates the Inst passed by the caller. If the caller relies on Inst being valid after the call, it might cause a surprise if the BB operations to insert the expanded sequence caused a reallocation that invalidated Inst pointer and other MCInsts pointers that the caller might be holding while calling that.

So we can add the Parent to the assert here (assert(Parent && ...)) plus a doc comment on the base declaration stating the reference may be invalidated. But ideally we would be stricter here - if now reverseBranchCondition is in the business of mutating the block, it must return the expansion as InstructionListType (as createCmpJE already does at AArch64MCPlusBuilder.cpp:2303) and let the caller splice it, keeping MCPlusBuilder out of CFG mutation.

https://github.com/llvm/llvm-project/pull/202998