Page MenuHomePhabricator

[mir-canon][NFCi] Adding opt arg to enable vreg renaming only mode.
AbandonedPublic

Authored by plotfi on Sep 2 2019, 12:38 AM.

Details

Summary

Similar to instnamer for llvm IR. I think it could be nice to have more readable string names. Might eventually add an option for a user defined string prefix for the vreg names instead of "namedVReg1234"

Diff Detail

Event Timeline

plotfi created this revision.Sep 2 2019, 12:38 AM

This is not a NFCI change - it is adding new functionality. Does it make sense perhaps to have this be a separate pass?

+1 for a separate pass. If we're going for an equivalent of -instnamer, I think it makes sense to keep the mode of use consistent.

I can imagine someone being thrown off by having to use

-run-pass mir-canonicalizer -canon-vregs-namer=1

versus something like

-run-pass=mirnamer

if they're used to just having to run -instnamer.

plotfi added a comment.Sep 3 2019, 9:29 AM

Sounds good.

paquette added inline comments.Sep 3 2019, 9:33 AM
llvm/test/CodeGen/MIR/AArch64/mirCanonCopyCopyProp.mir
1–2

Even though this test is pretty small, I think it would be better to have an explicit separate test for this.

(Then maybe you can use llvm/utils/update_mir_test_checks.py too, which is a nice time-saver. Not sure if it would work on this though?)

Also, is there any particular reason you don't pass -verify-machineinstrs on these tests? IIRC it always kicks in on debug builds, so it's nice to just always have it to prevent bot failures.

llvm/test/CodeGen/MIR/AArch64/mirCanonVRegnamer.mir
7–12

I don't think you need the IR if you pass in -mtriple aarch64-unknown-unknown to the llc line.

paquette added inline comments.Sep 3 2019, 9:40 AM
llvm/test/CodeGen/MIR/AArch64/mirCanonIdempotent.mir
11–19

I think this should be split out into a separate test as well.

Also, in the separate test, it looks like you can just axe everything beyond the FMOVDi since that's all that's tested anyway.

(Is there any particular reason this test is so long in the first place?)

plotfi marked 3 inline comments as done.Sep 3 2019, 10:47 AM
plotfi added inline comments.
llvm/test/CodeGen/MIR/AArch64/mirCanonCopyCopyProp.mir
1–2

actually, if I am going to do a new pass then year a separate test makes more sense.

I dont think there is a good reason the verifier isnt enabled here. I will add it.

llvm/test/CodeGen/MIR/AArch64/mirCanonIdempotent.mir
11–19

The reason it's so big I dont think is a very good one. I think I was just looking for some MIR code that had a ton of instructions like .* = FMOVDi 20 that could be moved to the top and sorted alphabetically.

llvm/test/CodeGen/MIR/AArch64/mirCanonVRegnamer.mir
7–12

Noted.

plotfi abandoned this revision.Sep 4 2019, 2:43 PM