This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Print "%vreg0" as "%0" in both MIR and debug output
ClosedPublic

Authored by thegameg on Nov 24 2017, 3:33 AM.

Details

Summary

As part of the unification of the debug format and the MIR format, avoid printing "vreg" for virtual registers (which is one of the current MIR possibilities).

As part of the unification of the debug format and the MIR format, avoid printing "vreg" for virtual registers (which is one of the current MIR possibilities).

Basically:

  • find . \( -name "*.mir" -o -name "*.cpp" -o -name "*.h" -o -name "*.ll" \) -type f -print0 | xargs -0 sed -i '' -E "s/%vreg([0-9]+)/%\1/g"
  • grep -nr '%vreg' . and fix if needed
  • find . \( -name "*.mir" -o -name "*.cpp" -o -name "*.h" -o -name "*.ll" \) -type f -print0 | xargs -0 sed -i '' -E "s/ vreg([0-9]+)/ %\1/g"
  • grep -nr 'vreg[0-9]\+' . and fix if needed

Diff Detail

Repository
rL LLVM

Event Timeline

thegameg created this revision.Nov 24 2017, 3:33 AM
MatzeB accepted this revision.Nov 27 2017, 11:16 AM

LGTM with a bunch of nitpicks. Thanks!

lib/CodeGen/DetectDeadLanes.cpp
24 ↗(On Diff #124151)

= use should move to the left

lib/CodeGen/PeepholeOptimizer.cpp
1699 ↗(On Diff #124151)

maybe use %[0-9]+ instead of just % in the example?

lib/CodeGen/RegisterCoalescer.cpp
1823–1824 ↗(On Diff #124151)

rename %X to %physreg_X to make it clear it is not a vreg as %Y.

1832–1834 ↗(On Diff #124151)

rename %Y to %physreg_Y to avoid confusion.

lib/CodeGen/SplitKit.cpp
1378–1380 ↗(On Diff #124151)

As we are touching this anyway: Rename to %0 and %1 as there is no reason for this to be 827/828 I believe.

lib/Target/AArch64/AArch64InstrInfo.cpp
2833 ↗(On Diff #124151)

(unrelated note: Wasn't this renamed to %xzr in your other commit?)

lib/Target/BPF/BPFISelDAGToDAG.cpp
549 ↗(On Diff #124151)

Use printReg()?

lib/Target/Hexagon/HexagonPeephole.cpp
16 ↗(On Diff #124151)

%166. (Maybe do a round of grep vreg[0-9] through the sourcecode to see if there are other places where people missed the % in front of vreg.

This revision is now accepted and ready to land.Nov 27 2017, 11:16 AM
thegameg added a subscriber: zer0.Nov 28 2017, 5:36 AM
qcolombet edited edge metadata.Nov 28 2017, 9:30 AM

This is a personal taste, but I find %0 less obvious than %vreg0.
I.e., I would be against this change and I would rather change the dump of .mir.

This is a personal taste, but I find %0 less obvious than %vreg0.
I.e., I would be against this change and I would rather change the dump of .mir.

I'd propose a compromise here:

  1. Switch the debug printing to use the same code as the MIRPrinter today (%[0-9]+)
  2. Once we have support for using arbitrary names for vregs we can choose any names when printing vregs. At that point change the debug printer to print %vreg[0-9]+

This is a personal taste, but I find %0 less obvious than %vreg0.
I.e., I would be against this change and I would rather change the dump of .mir.

+1
In addition to being more readable, I use vim and find it useful to place the cursor on a string to search for. It is much more likely that a string like vreg17 will match only what I care about than a string like 17. Of course, I could add the % to the search string but I'm not sure how to do that without requiring manual entry.
So I guess I just find the strings with vreg convenient with my workflow.

thegameg updated this revision to Diff 124756.Nov 29 2017, 8:13 AM
thegameg marked 8 inline comments as done.
thegameg edited the summary of this revision. (Show Details)

Address comments and rebase.

thegameg added inline comments.Nov 29 2017, 8:15 AM
lib/CodeGen/PeepholeOptimizer.cpp
1699 ↗(On Diff #124151)

I'l just keep %vreg = COPY %physreg.

lib/Target/AArch64/AArch64InstrInfo.cpp
2833 ↗(On Diff #124151)

Yep. I fixed some of my misses in lib/ and include/ in the commit (r319187).

lib/Target/BPF/BPFISelDAGToDAG.cpp
549 ↗(On Diff #124151)

I am using printReg here in D40421.

MatzeB accepted this revision.Nov 29 2017, 11:01 AM

Still LGTM. For the record: Quentin was fine with the compromise proposal in another email thread.

This revision was automatically updated to reflect the committed changes.