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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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? |
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? |
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? |
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. |
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp | ||
---|---|---|
279 | "the " is unnecessary in this sentence |
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?
- unsigned -> Register
- use const std::vector<MachineInstr *> & instead of call by value
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 |
- Use unique_ptr for VRegInfo
- Only run once DetectDeadLanes for each function
- Remove Seen and PHINodeLaneBitRecord
- Only call getRegClass once
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp | ||
---|---|---|
91–101 | When this function take VRNoV0RegClassID as input. the getSuperClasses will return as following order. AnyRegRegClassID 1 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. |
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? |
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? |
llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp | ||
---|---|---|
282 | Should this be std::unique_ptr<VRegInfo[]> since it points to an array? |