Page MenuHomePhabricator

[NFC][llvm][MIRVRegNamerUtils] Making some stylistic changes to MIRVRegNamerUtils.cpp
ClosedPublic

Authored by plotfi on Dec 8 2019, 10:22 PM.

Details

Summary

There are a couple places where I think some of the style could be a bit more modern or a little nicer looking so I made them.

Diff Detail

Event Timeline

plotfi created this revision.Dec 8 2019, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2019, 10:22 PM
compnerd added inline comments.Dec 9 2019, 7:45 AM
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
15–16

Why not const? The map itself isn’t modified.

plotfi updated this revision to Diff 232897.Dec 9 2019, 10:49 AM

Updated based on @compnerd's feedback.

plotfi marked 2 inline comments as done.Dec 9 2019, 10:49 AM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
15–16

ditto

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
18

I honestly don't think this is actually making this code easier to read (or modernizing) compared to the push_back (there's back_inserter, transform with lambda) compared to a simple one liner for loop. Maybe you want std::copy with the backinserter - but even that I'm conflicted on readability compared to a simple for loop here.

73

Again I find the for loop easy to read and the std::transform use case not so much.

plotfi marked 2 inline comments as done.Dec 9 2019, 11:58 AM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
18

I could std::copy references, but the transform here is to be more inline with the semantics of the previous for loop.

I don't know what the convention of LLVM is here - but I prefer the simple for loop with a push back vs a use of transform (which is not really transforming anything) which is confusing.

I don't know what the convention of LLVM is here - but I prefer the simple for loop with a push back vs a use of transform (which is not really transforming anything) which is confusing.

In one case, the transform is very subtle, and I can revert it back to the simple for loop. But in the cast with the hashing it is absolutely doing a transform and I would like to make the change.

I don't see how it's different for the hashing case.
On one side, you have

for (auto &Op: MI.uses())
  Ops.push_back(HashOperand(Op));

On the other side,

llvm::transform(MI.uses(), std::back_inserter(Ops),
                           [&HashOperand](const MachineOperand &MO) { return HashOperand(MO); });

I definitely don't think second case is easier to read, shorter than the first case and I really see no value in using transform here when essentially all it's doing is to just add a bunch of operands to a list.

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

plotfi marked 4 inline comments as done.Dec 9 2019, 1:41 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
18

I actually think this code here should be replaced with MachineRegisterInfo::replaceRegWith, and I am also not even sure if dropping the kill semantically makes sense. Was this taken from the old renamer or was this deliberately added for some specific shader here?

73

I'm sorry, but I just do not agree on this one. Here we specifically want to get a collection of hashes from a collection of uses. I think the transform is a lot more clear here.

plotfi updated this revision to Diff 232946.Dec 9 2019, 2:57 PM
plotfi marked an inline comment as done.

Update to make llvm::transform more clear than standard for-loop

plotfi marked 2 inline comments as done.Dec 9 2019, 3:00 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
18

Replaced this code with a call to MachineRegisterInfo::replaceRegWith. The if(!isDef)setIfKill(false) code I added in the original MIRCanon pass, and I am just not a fan of it here and I do not think it makes semantic sense. I think it could and should be dropped in a subsequent commit.

73

I think with this change, the new version is much more straight-forward. Are there any other concerns here? This is a NFC change. I think this should be changed here to be more inline with some of the more modern c++ and some of the STLExtras we are encouraged to use in LLVM.

plotfi added a comment.Dec 9 2019, 3:02 PM

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

bogner accepted this revision.Dec 9 2019, 3:19 PM

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

FWIW I'm not really convinced that std::transform is *ever* clearer or better any more since C++11 gave us auto and range-for.

In any case, the rest of this all seems like a nice clean up to me.

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
110–111

Slightly simpler to use a StringRef param and then this is just std::string Temp(Name.lower())

This revision is now accepted and ready to land.Dec 9 2019, 3:19 PM
plotfi added a comment.Dec 9 2019, 3:25 PM

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

FWIW I'm not really convinced that std::transform is *ever* clearer or better any more since C++11 gave us auto and range-for.

In any case, the rest of this all seems like a nice clean up to me.

Thanks for this. I understand what you mean. In any case, I dropped the other weird transform and am now reusing the method in MRI for renaming the vregs. I really want to drop that weird isDef/kill modification code that was added long ago, because I am not really sure the semantic implications of it and things were kind of an experiment when those 3 lines of code were put in long ago. Will land once I do the recommended StringRef change. @aditya_nandakumar @compnerd @bogner thank you for the review guys!

plotfi marked an inline comment as done.Dec 9 2019, 3:37 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
111–116

@aditya_nandakumar do you remember the reason that in the RC case we are using the lower case name but in the typed case we are creating a generic vreg but not lower casing the name? I don't remember this. Might put a comment in here for that.

plotfi updated this revision to Diff 232952.Dec 9 2019, 3:39 PM
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
111–116

I was only dealing with non generic vregs (post selection) - I didn't plan on using this mid GIsel pipeline - please update that as well while you can.

plotfi marked 2 inline comments as done.Dec 9 2019, 4:15 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
111–116

Sounds good. In that case I will designate this as "NFCi"

plotfi updated this revision to Diff 232983.Dec 9 2019, 6:34 PM
plotfi marked an inline comment as done.
plotfi marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.