This is an archive of the discontinued LLVM Phabricator instance.

Headers for basic blocks in control flow dot graphs
ClosedPublic

Authored by mark-sed on Jun 29 2023, 8:41 AM.

Details

Summary

This is 3. out of 3 changes to the CFG pass that aims to make graphs more readable and contain more information.

This change adds separators for basic block names, which makes it easier to find a basic block based on its name and separates it from the code. Currently there is also a chance that the basic block label will be present twice, that is in case the basic block has explicit numbering, this change does not contain this bug.

If all 3 changes will be accepted the final graphs should look as in the attached image:

Diff Detail

Event Timeline

mark-sed created this revision.Jun 29 2023, 8:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 8:41 AM
mark-sed requested review of this revision.Jun 29 2023, 8:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 8:41 AM
mark-sed edited the summary of this revision. (Show Details)Jun 29 2023, 8:43 AM
mark-sed updated this revision to Diff 536152.Jun 30 2023, 2:19 AM

Fixed polly test to contain the new header.

knaumov added inline comments.Jul 17 2023, 4:56 PM
llvm/include/llvm/Analysis/CFGPrinter.h
149

Haven't you changed the logic of the OS argument here? It seems that now it is always empty.

154

Nit: Typo in "Prepend"

mark-sed added inline comments.Jul 18 2023, 1:29 AM
llvm/include/llvm/Analysis/CFGPrinter.h
149

So the previous code was a cause for a bug, where basic blocks with numbered label got their label displayed twice in the CFG. getName() will be empty in case of implicit and also explicit numbered name, but the explicit will get it inserted in the HandleBasicBlock. So the solution here is to prepend the name only if the HandleBasicBlock did not already do it.

mark-sed updated this revision to Diff 541393.Jul 18 2023, 1:31 AM

Fixed typo in a comment.

mark-sed marked an inline comment as done.Jul 18 2023, 1:32 AM
mark-sed added inline comments.
llvm/include/llvm/Analysis/CFGPrinter.h
154

Fixed.

I think a better structure would be:

  • Don't emit the block name in HandleBasicBlock. Instead of printing the BB, just iterate through the instructions and print them 1 by 1.
  • Unconditionally prepend the block name in CompleteNodeLabelString.
llvm/include/llvm/Analysis/CFGPrinter.h
138

Please, avoid unrelated whiteline changes

mark-sed updated this revision to Diff 548598.Aug 9 2023, 6:19 AM
mark-sed marked an inline comment as done.

Added changes requested by apilipenko, where the HandleBasicBlock now prepends the BB name and then appends instructions instead of using << operator on the whole BB.

apilipenko accepted this revision.Aug 15 2023, 2:47 PM
This revision is now accepted and ready to land.Aug 15 2023, 2:47 PM
mark-sed updated this revision to Diff 551114.Aug 17 2023, 6:28 AM

Fixed tests that were failing. The new implementation does not leave extra whitespace.

This revision was landed with ongoing or failed builds.Aug 17 2023, 7:55 AM
This revision was automatically updated to reflect the committed changes.