[RISCV] Handle non uimm5 VL constants in isVLKnownLE #156639
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4796,8 +4796,21 @@ unsigned RISCV::getDestLog2EEW(const MCInstrDesc &Desc, unsigned Log2SEW) { | |
| return Scaled; | ||
| } | ||
|
|
||
| static std::optional<int64_t> getEffectiveImm(const MachineOperand &MO, | ||
| const MachineRegisterInfo *MRI) { | ||
| assert(MO.isImm() || MO.getReg().isVirtual()); | ||
| if (MO.isImm()) | ||
| return MO.getImm(); | ||
| MachineInstr *Def = MRI->getVRegDef(MO.getReg()); | ||
| if (Def->getOpcode() == RISCV::ADDI && | ||
| Def->getOperand(1).getReg() == RISCV::X0) | ||
|
||
| return Def->getOperand(2).getImm(); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| /// Given two VL operands, do we know that LHS <= RHS? | ||
| bool RISCV::isVLKnownLE(const MachineOperand &LHS, const MachineOperand &RHS, | ||
| const MachineRegisterInfo *MRI) { | ||
| if (LHS.isReg() && RHS.isReg() && LHS.getReg().isVirtual() && | ||
| LHS.getReg() == RHS.getReg()) | ||
| return true; | ||
|
|
@@ -4807,9 +4820,11 @@ bool RISCV::isVLKnownLE(const MachineOperand &LHS, const MachineOperand &RHS) { | |
| return true; | ||
| if (LHS.isImm() && LHS.getImm() == RISCV::VLMaxSentinel) | ||
| return false; | ||
| std::optional<int64_t> LHSImm = getEffectiveImm(LHS, MRI), | ||
|
Collaborator
There was a problem hiding this comment. Are you sure the new logic works properly with VLMaxSentinel? In particular, what if you get an ADDI which happens to encode -1? I think this is correct, just flagging it for extra consideration.
Contributor
Author
There was a problem hiding this comment. I originally checked for VLMaxSentinel in ADDI, but undid this in 8072205. Since in the places where we check for VLMaxSentinel we don't seem to check for ADDIs, we only consider immediate operands. E.g. in RISCVInsertVSETVLI if (VLOp.isImm()) {
int64_t Imm = VLOp.getImm();
// Convert the VLMax sentintel to X0 register.
if (Imm == RISCV::VLMaxSentinel) { ...
Member
There was a problem hiding this comment.
I guess the question here is whether we ever generate ADDI with -1. Because if we do, then I think even for places that do not check for VLMaxSentinel might behave incorrectly. For instance, in VLOpt it will aggregate user VLs and pick the largest one. Without checking if the effective imm is VLMaxSentinel, ADDI with -1 will never be picked when it should be.
Collaborator
There was a problem hiding this comment.
I think we're okay here, and it would in fact be wrong to special case ADDI xN, x0, -1. The current immediate field is essentially a union of two states - actual immediate which are in the range of 0-31 and the VLMaxSentinel which represents the symbolic VLMAX value. We just happen to use the value -1 (which doesn't correspond to a valid immediate) for this purpose. Now, if we did special case ADDI -1 as VLMaxSentinel, we'd probably never notice. -1 interpreted an unsigned XLEN is going to be way larger than any possible VLMAX, and given the rules of vsetvli would result in VL=VLMAX anyways. So the difference is probably not visible. (In case it's not clear, I'm agreeing with Luke, just explaining my reasoning on how I got there.) |
||
| RHSImm = getEffectiveImm(RHS, MRI); | ||
| if (!LHSImm || !RHSImm) | ||
| return false; | ||
| return LHSImm <= RHSImm; | ||
| } | ||
|
|
||
| namespace { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this a predicate. We have a lot of code doing the same judgement, like
isLoadImm.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just reused isLoadImm instead in a6c215c