章明 via llvm-dev
2017-Nov-02 13:49 UTC
[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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20171102/8d7a9fa7/attachment.html>
Sameer AbuAsal via llvm-dev
2017-Nov-02 19:13 UTC
[llvm-dev] Possible bugs in class ARMConstantIslands
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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20171102/7cccd3ac/attachment.html>