[CodeGen] Fix lpad padding at section start after empty block #112595
Conversation
pzfl
requested review from
dhoekwater and
WenleiHe
and removed request for
dhoekwater
|
I have moved the check directly to |
| @@ -265,8 +265,22 @@ void llvm::sortBasicBlocksAndUpdateBranches( | |||
| // zero implies "no landing pad." This function inserts a NOP just before the EH | |||
| // pad label to ensure a nonzero offset. | |||
| void llvm::avoidZeroOffsetLandingPad(MachineFunction &MF) { | |||
| std::optional<MBBSectionID> CurrentSection; | |||
| bool SectionEmpty = true; | |||
| auto IsFirstNonEmptyBBInSection = [&](const MachineBasicBlock &MBB) { | |||
There was a problem hiding this comment.
This function may give false positives in the following situation:
bb0 (bbsections 0)
bb1 (bbsections cold)
bb2 (bbsections 0)
If false positives are fine (since we're just inserting a nop), then you can disregard this feedback. Otherwise, you may want to do something like
llvm::DenseSet<MBBSectionID> SectionsWithNonEmptyBlocks;
auto IsFirstNonEmptyBBInSection = [&](const MachineBasicBlock &MBB) {
if (MBB.empty() || SectionsWithNonEmptyBlocks.contains(MBB.getSectionID()))
return false;
SectionsWithNonEmptyBlocks.insert(MBB.getSectionID());
There was a problem hiding this comment.
Good catch! I was under the impression that basic blocks are assumed to be sorted. Both callsites avoidZeroOffsetLandingPad are preceded by a call to sortBasicBlocksAndUpdateBranches. However, I see now that finishAdjustingBasicBlocksAndLandingPads sorts only by section type, but not by section number. I am not sure how section numbers are used.
There was a problem hiding this comment.
Ahh, no you're right, sortBasicBlocksAndUpdateBranches guarantees the basic block sections will be contiguous. Looks good!
pzfl
force-pushed
the
pzfl/o/lpad0
branch
3 times, most recently
from
9ab0175 to
ea3939c
Compare
| @@ -265,8 +265,22 @@ void llvm::sortBasicBlocksAndUpdateBranches( | |||
| // zero implies "no landing pad." This function inserts a NOP just before the EH | |||
| // pad label to ensure a nonzero offset. | |||
| void llvm::avoidZeroOffsetLandingPad(MachineFunction &MF) { | |||
| std::optional<MBBSectionID> CurrentSection; | |||
| bool SectionEmpty = true; | |||
| auto IsFirstNonEmptyBBInSection = [&](const MachineBasicBlock &MBB) { | |||
There was a problem hiding this comment.
Ahh, no you're right, sortBasicBlocksAndUpdateBranches guarantees the basic block sections will be contiguous. Looks good!
|
Hi @dhoekwater, sorry for the delay. I updated the patch to only keep track of the section again. If this looks good to you, could you please merge this (I do not have write permission)? |
If a landing pad is at the very start of a split section, it has to be padded by a nop instruction. Otherwise its offset is marked as zero in the LSDA, which means no landing pad (leading it to be skipped). LLVM already handles this. If a landing pad is the first machine block in a section, a nop is inserted to ensure a non-zero offset. However, if the landing pad is preceeded by an empty block, the nop would be omitted. To fix this, this patch checks not whether a block is the first in section, but instead whether it is the first *non-empty* block in the section.
If a landing pad is at the very start of a split section, it has to be padded by a nop instruction. Otherwise its offset is marked as zero in the LSDA, which means no landing pad (leading it to be skipped).
LLVM already handles this. If a landing pad is the first machine block in a section, a nop is inserted to ensure a non-zero offset. However, if the landing pad is preceeded by an empty block, the nop would be omitted.
To fix this, this patch adds a field to machine blocks indicating whether this block contains the first instruction in its section. This variable is then used to determine whether to emit the padding.