Page MenuHomePhabricator

[mir-canon][NFC] Move MIR Vreg renaming code to separate file for better reuse.
ClosedPublic

Authored by plotfi on Sep 3 2019, 10:41 AM.

Details

Summary

This change is actually NFC. I will post a pass that uses the virtual register naming code from MIRCanonicalizatioPass to only rename vregs for the purposes of making MIR tests more readable.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.Sep 3 2019, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2019, 10:41 AM

This is just code movement, but still a couple of nits/questions.

Only thing I think that I'd really like to see here is some documentation in the header file for functions, classes, etc. If this is going to be shared between some passes, I think it makes sense to document what each thing does, and how these things fit together.

And, since we're moving it all anyway, I guess it'd be worth making function/variable names fit the standard style while we're here.

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
15–20 ↗(On Diff #218488)
  • Move Doxygen comment to the header?
  • s/An/A/
21 ↗(On Diff #218488)

Now that this isn't fully tied to the canonicalizer, I think it would make sense to give it a more descriptive name?

E.g. findInstructionsWithSideEffects or something?

51 ↗(On Diff #218488)

Same thing here? Basically, anything with "candidate" makes less sense outside the context of a specific pass.

(Maybe it would make sense to pack all of this into a Renamer struct/class or something. I don't have any strong opinions there though.)

llvm/lib/CodeGen/MIRVRegNamerUtils.h
29 ↗(On Diff #218488)

I guess from the name, in concept it's obvious what this class *is*, but it would be nice to have a Doxygen comment documenting what its purpose is etc.

30 ↗(On Diff #218488)

While we're here, might as well rename things to fit the style guide?

e.g. Type, Reg, etc.

31 ↗(On Diff #218488)

Use Register instead of unsigned? Or is this your own thing? (We started using Register instead of unsigned for registers recently.)

50 ↗(On Diff #218488)

This class could use a Doxygen comment as well. It's a little less obvious what it does.

57 ↗(On Diff #218488)

skipVRegs

63 ↗(On Diff #218488)

Why 1000?

100 ↗(On Diff #218488)

getVRegRenameMap

plotfi marked an inline comment as done.Sep 3 2019, 2:17 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.h
29 ↗(On Diff #218488)

Actually at second glance, I dont think these need to be exposed outside of the .cpp file.

plotfi updated this revision to Diff 218532.Sep 3 2019, 2:18 PM

Moving the guts of the candidate walk loop into the reusable MIRVRegNamer.cpp library

plotfi marked an inline comment as done.Sep 3 2019, 3:25 PM
plotfi updated this revision to Diff 218557.Sep 3 2019, 3:40 PM
plotfi marked 5 inline comments as done.
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
21 ↗(On Diff #218488)

I've decided to isolate these from the header file. So for now the names can remain without confusion.

51 ↗(On Diff #218488)

Agreed. I can consider changing everything named "candidate" to something else, but I'd rather do it in another commit. I've wrapped most of the functions into an anonymous namespace in the cpp file at this point.

plotfi updated this revision to Diff 218559.Sep 3 2019, 4:06 PM
plotfi marked 5 inline comments as done.

addressing @paquette's suggestions.

plotfi updated this revision to Diff 218560.Sep 3 2019, 4:08 PM
plotfi marked 2 inline comments as done.
plotfi updated this revision to Diff 218566.Sep 3 2019, 4:39 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.h
63 ↗(On Diff #218488)

It's used as a default value for skipping indices between BBs and side-effect tree roots. I've made it an optional parameter of the NamedVRegCursor.

paquette accepted this revision.Sep 4 2019, 10:34 AM

LGTM with some nits on comments.

llvm/lib/CodeGen/MIRVRegNamerUtils.h
35 ↗(On Diff #218566)

Judging by the SkipGapSize comment below, I think you wanted to document this?

I guess it's probably a good idea anyway, considering you can initialize it etc?

45–46 ↗(On Diff #218566)

s/SkipGapSize/skipVRegs/

This revision is now accepted and ready to land.Sep 4 2019, 10:34 AM
plotfi updated this revision to Diff 218774.Sep 4 2019, 12:57 PM
plotfi marked 4 inline comments as done.

More comments.

This revision was automatically updated to reflect the committed changes.