Page MenuHomePhabricator

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

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



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

Unit TestsFailed

470 mslinux > LLVM.CodeGen/PowerPC::aix-xcoff-reloc-symb.mir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc-ibm-aix-xcoff -x mir -verify-machineinstrs -start-after=lazy-machine-block-freq -filetype=obj -o /mnt/disks/ssd0/agent/llvm-project/build/test/CodeGen/PowerPC/Output/aix-xcoff-reloc-symb.mir.tmp.o < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-symb.mir
260 mslinux > LLVM.DebugInfo/MIR/ARM::subregister-full-piece.mir
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -start-after=livedebugvalues -filetype=obj -o - /mnt/disks/ssd0/agent/llvm-project/llvm/test/DebugInfo/MIR/ARM/subregister-full-piece.mir | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-dwarfdump - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/DebugInfo/MIR/ARM/subregister-full-piece.mir
130 mswindows > LLVM.CodeGen/PowerPC::aix-xcoff-reloc-symb.mir
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llc.exe -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc-ibm-aix-xcoff -x mir -verify-machineinstrs -start-after=lazy-machine-block-freq -filetype=obj -o C:\ws\w16c2-1\llvm-project\premerge-checks\build\test\CodeGen\PowerPC\Output\aix-xcoff-reloc-symb.mir.tmp.o < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\aix-xcoff-reloc-symb.mir
170 mswindows > LLVM.DebugInfo/MIR/ARM::subregister-full-piece.mir
Script: -- : 'RUN: at line 2'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llc.exe -start-after=livedebugvalues -filetype=obj -o - C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\MIR\ARM\subregister-full-piece.mir | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llvm-dwarfdump.exe - | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\MIR\ARM\subregister-full-piece.mir

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

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?


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

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.


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

snehasish added inline comments.Oct 15 2020, 10:27 AM

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


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


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


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


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

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.


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

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


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.


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

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

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.