This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add a pass to merge moving parameter registers instructions for Zcmp
ClosedPublic

Authored by VincentWu on May 11 2023, 6:59 PM.

Details

Summary

This patch adds a pass to generate cm.mvsa01 & cm.mva01s.

RISCVMoveOptimizer.cpp which combines two mv inst into one cm.mva01s or cm.mva01s.

Diff Detail

Event Timeline

VincentWu created this revision.May 11 2023, 6:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 6:59 PM
VincentWu requested review of this revision.May 11 2023, 6:59 PM
kito-cheng added inline comments.May 12 2023, 12:17 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1296–1305 ↗(On Diff #521517)

This change seems right, but that should be a separated patch :)

craig.topper added inline comments.May 12 2023, 12:42 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
1296–1305 ↗(On Diff #521517)

Do we ever create such instructions?

VincentWu updated this revision to Diff 521878.May 13 2023, 1:29 AM
VincentWu marked 2 inline comments as done.

address comments

craig.topper added inline comments.May 15 2023, 5:58 PM
llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp
63

const DestSourcePair &RegPair

68

Join the two ifs with &&

69

SR07RegClass.contains(Source) should work I think

75

const DestSourcePair &RegPair

81

SR07RegClass.contains(Dest)

81

Join the two ifs with &&

135

Can you pass the DestSourcePair from the caller?

174

Why are the available checks different between the two cases? Aren't we always moving the paired copy earlier in the basic block to the location of the first copy. So shouldn't the register checks be the same?

193

There is no reason to shorten Move to Mov in this function name. And function names should typically start with a verb. optimizeBlock is probably a better name

230

Use Fn.getSubtarget<RISCVSubtarget>()

231

Drop curly braces

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
351

Should we disable for -O0?

craig.topper added inline comments.May 15 2023, 6:00 PM
llvm/test/CodeGen/RISCV/cm_mvas_mvsa.ll
14

Shouldn't we have more tests to cover cases where the copies aren't next to each other and cases where registers are modified/used in between?

evandro removed a subscriber: evandro.May 17 2023, 3:56 PM
VincentWu marked 12 inline comments as done.

address comments

craig.topper added inline comments.May 31 2023, 1:30 PM
llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp
32

Subtarget doesn't need to be a member of the class. It's only used in runOnMachineFunction

111

Put ARegInFirstPair == RISCV::X10 in a variable instead of repeating it 4 times.

163

Why don't we need to check UsedRegUnits.available(DestReg) like we do for CM_MVSA01?

169

Why don't we need to we check ModifiedRegUnits.available(DestReg) like we do for RISCV::CM_MVA01S?

188

This function name is just a shortened form of the pass name. It should be a verb phrase as documented here https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

VincentWu updated this revision to Diff 528303.Jun 5 2023, 12:35 AM
VincentWu marked 6 inline comments as done.

address comments

craig.topper added inline comments.Jun 5 2023, 6:50 PM
llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp
182

Function names should start with a lower case letter

204

could -> can

232

Drop the curly braces

VincentWu updated this revision to Diff 530424.Jun 12 2023, 1:15 AM
VincentWu marked 3 inline comments as done.

address comment

craig.topper added inline comments.Jun 13 2023, 2:34 PM
llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp
9

This description should mention Zcmp

223

This static_cast isn't necessary. RISCVSubtarget has an implementation of getInstrInfo that returns RISCVInstrInfo *

craig.topper added inline comments.Jun 13 2023, 3:47 PM
llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp
19

We're inconsistently using "move optimizer" and "move merging". Can we pick one?

VincentWu updated this revision to Diff 532349.Jun 16 2023, 7:19 PM
VincentWu marked 3 inline comments as done.

address comments

This revision is now accepted and ready to land.Jun 17 2023, 6:59 PM
This revision was landed with ongoing or failed builds.Jun 21 2023, 12:42 AM
This revision was automatically updated to reflect the committed changes.