[llvm-dev] Possible bugs in class ARMConstantIslands

[章明 via llvm-dev]

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 
>