Page MenuHomePhabricator

[RISCV] Add new pass to transform undef to pseudo for vector values.
Needs ReviewPublic

Authored by BeMg on Jul 14 2022, 12:38 AM.

Details

Summary

RISC-V vector instruction has register overlapping constraint for certain
instructions, and will cause illegal instruction trap if violated, we use
early clobber to model this constraint, but it can't prevent register allocator
allocated same or overlapped if the input register is undef value, so convert
IMPLICIT_DEF to temporary pseudo could prevent that happen, it's not best way
to resolve this. Ideally we should model the constraint right, but before we
model the constraint right, it's the approach to prevent that happen.

See also: https://github.com/llvm/llvm-project/issues/50157

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
BeMg updated this revision to Diff 478437.Nov 28 2022, 6:47 PM

Update testcase

pseudo is misspelled in the title

craig.topper added inline comments.Nov 29 2022, 4:41 PM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
2

pesudo -> pseudo

11

"pesudo" -> "pseudo" in two places

19

pesudo -> pseudo

19

latter -> later

246

Pesudo -> Pseudo

247

regitser -> register

289

Candidata -> Candidate?

llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
29

Are we treating insert_subreg for segment load tuples the same as inserting a small LMUL into a wider LMUL?

craig.topper added inline comments.Nov 29 2022, 4:44 PM
llvm/test/CodeGen/RISCV/rvv/subregister-undef-early-clobber-vrm4.mir
118 ↗(On Diff #478437)

Why is there an PseudoRVVInitUnde pseudo in the IR before the pass runs?

craig.topper added inline comments.Nov 29 2022, 4:54 PM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
74

You can pass Register by value

106

origin -> original

111

Can't we do something like

VRRegClass.hasSubClassEq(MRI->getRegClass(R)) ||
VRM2RegClass.hasSubClassEq(MRI->getRegClass(R)) ||
VRM4RegClass.hasSubClassEq(MRI->getRegClass(R)) ||
VRM8RegClass.hasSubClassEq(MRI->getRegClass(R))

why do we need to use getVRLargestSuperClass?

BeMg retitled this revision from [RISCV] Add new pass to transform undef to pesudo for vector values. to [RISCV] Add new pass to transform undef to pseudo for vector values..Nov 30 2022, 12:40 AM
BeMg updated this revision to Diff 478877.Nov 30 2022, 3:15 AM
  1. Fix spell
  2. Update isVectorRegClass
BeMg updated this revision to Diff 479164.Nov 30 2022, 9:45 PM
  1. Rplace PseudoRVVInitUndef with VLE in MIR test
  2. Move MIR test into precommit
BeMg added inline comments.Dec 13 2022, 6:18 AM
llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
29

This pass doesn't consider segment load as instruction that assign sub-register.

The following Insert_subreg work like put %5:vrm2 into %6:vrm4

%1:vrm4 = IMPLICIT_DEF
%5:vrm2 = PseudoVLE32_V_M2 killed %4, 0, 5 /* e32 */
%6:vrm4 = INSERT_SUBREG %1, %5, %subreg.sub_vrm2_0

Do we should treat vloxseg2ei32 as INSERT_SUBREG in this patch?

craig.topper added inline comments.Dec 13 2022, 2:40 PM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
172

Why passing Insts by value? That will make a copy but it doesn't look like we need a copy.

182

Why passing Insts by value? That will make a copy but it doesn't look like we need a copy.

183

unsigned -> Register

197

unsigned -> Register

197

Why passing Insts by value? That will make a copy but it doesn't look like we need a copy.

200

unsigned -> Register

299

unsigned -> Register

300

unsigned -> Register

313

createVirtualRegister returns Register not unsigned

317

createVirtualRegister returns Register not unsigned

llvm/test/CodeGen/RISCV/regalloc-last-chance-recoloring-failure.ll
29

Nevermind, I didn't realize this test had so many undef and poison operands. I suspect llvm-reduce or bugpoint. I dislike tests with undef/poison operands. It makes things very fragile. It would be legal for DAG combine to delete a large portion of this test.

craig.topper added inline comments.Dec 13 2022, 2:52 PM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
279

"the " is unnecessary in this sentence

craig.topper added inline comments.Dec 13 2022, 2:54 PM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
28

"is be occupied" -> "is occupied"

Is it possible to insert the PseudoRVVInitUndef instructions after we've left SSA and the LiveIntervals have been built. Would that make it easier to find the undef lanes?

BeMg updated this revision to Diff 482694.Dec 13 2022, 8:14 PM
  1. unsigned -> Register
  2. use const std::vector<MachineInstr *> & instead of call by value
BeMg updated this revision to Diff 483741.Dec 17 2022, 4:25 AM

Reuse DetectDeadLanes pass info for undefined subregister

BeMg updated this revision to Diff 484196.Dec 20 2022, 2:08 AM

Move DetectDeadLanes pass change into another patch

craig.topper added inline comments.Dec 21 2022, 10:35 AM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
202

Isn't this calculating for the entire function? But handleSubReg is called for individual instructions. So we'll be recomputing information right?

This could certainly use some new MIR tests. I didn't look super closely but I'm not sure you're correctly handling undef vs. not-undef subreg defs

llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
54

DenseMap

91–101

I don't understand the point of this function, getSuperClasses is already sorted by largest. You can just take the first?

106

Could this use the new getPhysRegBaseClass?

108

Don't call getRegClass for each use

142

Reg.isVirtual()

201

unique_ptr

BeMg updated this revision to Diff 484791.Dec 22 2022, 3:36 AM
  1. Use unique_ptr for VRegInfo
  2. Only run once DetectDeadLanes for each function
  3. Remove Seen and PHINodeLaneBitRecord
  4. Only call getRegClass once
BeMg marked 4 inline comments as done.Dec 22 2022, 3:39 AM
BeMg added inline comments.
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
91–101

When this function take VRNoV0RegClassID as input. the getSuperClasses will return as following order.

AnyRegRegClassID 1
VMRegClassID 20
VRRegClassID 21 <- stop here
...

This patch only want those four RegClass as result.

106

Could this hook use for virtual register? It seem only for physical register but this pass run before register allocation.

202

Yes, you're right. we don't need recompute this info for each instruction. It call only once now.

BeMg updated this revision to Diff 485949.Jan 3 2023, 3:15 AM

Add more subregister testcase

craig.topper added inline comments.Tue, Jan 3, 9:31 AM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
40

Are there any maps in the code anymore?

llvm/test/CodeGen/RISCV/O3-pipeline.ll
110

If I understand correctly, we're effectively running DetectDeadLanes inside of RISCV init undef pass and then running the real DetectDeadLanes pass which won't do anything because we already did it?

craig.topper added inline comments.Tue, Jan 3, 10:35 AM
llvm/test/CodeGen/RISCV/O3-pipeline.ll
110

Can we run DetectDeadLanes, then run our pass and just use the portion of the DetectDeadLanes that computes the Lane Masks in our pass?

BeMg updated this revision to Diff 486465.Wed, Jan 4, 8:55 PM
  1. Move init undef pass after DetectDeadLanes
  2. Remove <map>
craig.topper added inline comments.Wed, Jan 4, 9:09 PM
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp
282

Should this be std::unique_ptr<VRegInfo[]> since it points to an array?

BeMg updated this revision to Diff 486467.Wed, Jan 4, 9:26 PM

Update std::unique_ptr<VRegInfo> with std::unique_ptr<VRegInfo[]>

BeMg updated this revision to Diff 487364.Mon, Jan 9, 3:01 AM

rebase

BeMg updated this revision to Diff 487663.Mon, Jan 9, 8:43 PM

rebaseY

luke957 removed a subscriber: luke957.Sat, Jan 21, 7:51 AM
BeMg updated this revision to Diff 494165.Wed, Feb 1, 10:02 PM

Use D141993 as DetectDeadLane user interface