Page MenuHomePhabricator

Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty.
ClosedPublic

Authored by rahmanl on Oct 14 2020, 1:28 PM.

Details

Summary

Sometimes in unoptimized code, we have dangling unreachable basic blocks with no predecessors. Basic block sections should be emitted for those as well. Without this patch, the included test fails with a fatal error in AsmPrinter::emitBasicBlockEnd.

Diff Detail

Event Timeline

rahmanl created this revision.Oct 14 2020, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 1:28 PM
rahmanl requested review of this revision.Oct 14 2020, 1:28 PM
rahmanl edited the summary of this revision. (Show Details)Oct 14 2020, 1:28 PM
tmsriram added inline comments.Oct 14 2020, 1:54 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3129–3133

This is the only place that EmitMBBlabel is checked. I don't see why you can't put that condition just directly after the if?

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll
4

There are no CHECK: statements?

rahmanl updated this revision to Diff 298298.Oct 14 2020, 10:05 PM
  • Fix the condition for emitting basic block label.
rahmanl updated this revision to Diff 298299.Oct 14 2020, 10:10 PM
  • Remove redundant CHECK check-prefix.
rahmanl marked 2 inline comments as done.Oct 14 2020, 10:11 PM

Thanks for the comments @tmsriram.

tmsriram added inline comments.Oct 14 2020, 10:18 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3129–3133

I think this would require a lot more comments. I see that you have flipped the if-else here, is there any reason you did that. Plus, there are a lot more checks than earlier. Could you add more comments that explain what is going on here? Thanks.

llvm/lib/CodeGen/MachineBasicBlock.cpp
270

Is this the same as getParent()->front() == this?

snehasish added inline comments.Oct 15 2020, 10:27 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3129–3133

+1 perhaps even consider refactoring it out into a static helper function.

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll
18

Should we also check for the appropriate CFI directives?

rahmanl updated this revision to Diff 298463.Oct 15 2020, 2:12 PM
  • Add a private member function shouldEmitLabelForBasicBlock and call it to decide if basic blocks require symbols.
rahmanl marked 3 inline comments as done.Oct 15 2020, 2:24 PM

Thank you for your helpful comments @tmsriram and @snehasish

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3129–3133

I refactored this into a private member function. The reason is that isBlockOnlyReachableByFallthrough is a virtual member function.

llvm/lib/CodeGen/MachineBasicBlock.cpp
270

Yes, except with a &:
&getParent()->front() == this
I find the current version more readable.

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll
18

I intentionally want to limit the test to checking labels and sections only. The reason is that the CFI directive of this block may be removed since it has no real instructions. However, I think that may be a later follow-up with a separate test. Please let me know if you have a good reason.

tmsriram added inline comments.Oct 15 2020, 2:52 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3129–3133

Should we keep the same order as before? Any reason to flip it? I don't have a strong opinion but the change is simpler.

3179
3181

"assume different names under various circumstances" , please rephrase.

3184
3185
3189
rahmanl updated this revision to Diff 299182.Oct 19 2020, 2:54 PM
rahmanl marked 2 inline comments as done.
  • Better comments.
rahmanl marked 5 inline comments as done.Oct 19 2020, 2:57 PM

Thanks again for the comments @tmsriram

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3129–3133

Thanks for the comment. I think initially the condition was reversed due to the simpler logic of the "false" direction. Since we now use a helper function to extract the condition, we can do it the natural way. I can change this to if (!shouldEmitLabelForBasicBlock(MBB)) and reverse the two directions, but don't see the value in that.

3181

Removed the phrase.

tmsriram accepted this revision.Oct 19 2020, 3:04 PM

LGTM, thanks.

This revision is now accepted and ready to land.Oct 19 2020, 3:04 PM
MaskRay added inline comments.Oct 19 2020, 3:29 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3178

The coding standard does not replicate the function name for a comment. Some old code uses does not conform to the standard

rahmanl updated this revision to Diff 300824.Oct 26 2020, 4:12 PM
rahmanl marked 2 inline comments as done.
  • Remove function comment for AsmPrinter::shouldEmitLabelForBasicBlock.
rahmanl added inline comments.Oct 26 2020, 4:14 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3178

Removed the comment.

This revision was landed with ongoing or failed builds.Oct 26 2020, 4:16 PM
This revision was automatically updated to reflect the committed changes.