llvm dev - Nov 2017 - Possible bugs in class ARMConstantIslands

A moment ago, I realized that ImmBranches is already correctly updated after the
new conditional branch is added.
The code is as follows:

// Insert a new conditional branch and a new unconditional branch.
// Also update the ImmBranch as well as adding a new entry for the new branch.
BuildMI(MBB, DebugLoc(), TII->get(MI->getOpcode()))
  .addMBB(NextBB).addImm(CC).addReg(CCReg);
Br.MI = &MBB->back();
BBInfo[MBB->getNumber()].Size += TII->getInstSizeInBytes(MBB->back());

Althrough the original conditional branch is removed, the corresponding
ImmBranch object remains in ImmBranches.
After the new conditional branch is added, the ImmBranch object of the original
conditional branch is updated to associate with the new conditional branch.
Since the conditional branches have the same opcode, other members of the
ImmBranch object, including MaxDisp, isCond and UncondBr, are already correct
and do not need updating. Therefore, the ImmBranch object is correct for the new
conditional branch, and the new conditional branch will be seen during
subsequent iterations.

I am sorry for the false alarm.

> -----Original Messages-----
> From: Sameer AbuAsal via llvm-dev <llvm-dev at lists.llvm.org>
> To: '章明' <editing at zju.edu.cn>
> Cc: llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] Possible bugs in class ARMConstantIslands
> Message-ID: <37cc01d3540e$9c1c1dc0$d4545940$@codeaurora.org>
> Content-Type: text/plain; charset="utf-8"
> 
> Hi, 
> 
>  
> 
> I think these are actually real bugs, we have a discussion about them here:
https://reviews.llvm.org/D38918
> 
> If you have a test case you can add please do. I managed to get this to
break in an out of tree build and need to come up with a test case for upstream
to get the patch in.
> 
>  
> 
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of ??
via llvm-dev
> Sent: Thursday, November 2, 2017 6:49 AM
> To: llvm-dev at lists.llvm.org
> Subject: [llvm-dev] Possible bugs in class ARMConstantIslands
> 
>  
> 
> Hi, there! 
> 
>  
> 
> When reading code of ARMConstantIslands::fixupConditionalBr, I found two
possible bugs.
> 
>  
> 
> The first bug is related to the CFG. 
> 
> If the basic block that contains the conditional branch in question is
split, splitBlockBeforeInstr updates the CFG so that the basic block BEFORE the
split point has only one successor, that is, the new basic block AFTER the split
point. However, later, a new conditional branch, which targets the destination
of the original conditional branch in question, is added to the basic block
BEFORE the split point. This new conditional branch makes the destination of the
original conditional branch a successor of the basic block BEFORE the split
point. In addition, the destination of the original conditional branch may no
longer be a successor of the basic block AFTER the split point. The CFG is not
updated accordingly to reflect these facts.
> 
>  
> 
> The second bug is that if a new conditional branch, which targets NextBB,
i.e., the basic block next to MBB, is added to MBB, the new conditional branch
is not added to ImmBranches.
> 
> Note that after the new conditional branch is added to MBB, an
unconditional branch is always added to MBB, making MBB a water block.
> 
> If a large number of constant pool entries are added after the water block,
NextBB may be moved out of the range of the new conditional branch. But the pass
will not be able to fix this, since it does not see the new conditional branch
in ImmBranches.
> 
>  
> 
> I haven't performed any test to confirm these bugs. 
> 
> Are these really bugs, or did I miss something? 
> 
>  
> 
> Ming Zhang 
>