This is an archive of the discontinued LLVM Phabricator instance.

Differential D119517

Revert "[RISCV] Fix a vsetvli insertion bug involving loads/stores." and "[RISCC] Add missing words to comment. NFC"
ClosedPublic

Authored by craig.topper on Feb 10 2022, 9:46 PM.

Details

Reviewers
frasercrmck
rogfer01
jacquesguan
kito-cheng
khchen
arcbbb
Commits
rGf35ac872b822: Revert "[RISCV] Fix a vsetvli insertion bug involving loads/stores." and "…
Summary

This reverts commit f943c58cae2480755cecdac5be832274f238df93.
and commit 7eb781072744b31a60e82b5a5903471032d4845f.

This introduced a new bug that appears to be easier to hit.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 10 2022, 9:46 PM
Herald added subscribers: VincentWu, luke957, achieveartificialintelligence and 24 others. · View Herald TranscriptFeb 10 2022, 9:46 PM
craig.topper requested review of this revision.Feb 10 2022, 9:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 9:46 PM
Herald added subscribers: • pcwang-thead, eopXD, MaskRay. · View Herald Transcript
Harbormaster completed remote builds in B148908: Diff 407766.Feb 10 2022, 9:47 PM
craig.topper added a parent revision: D119516: [RISCV] Add test case for a vsetvli insertion bug found after D118667..Feb 10 2022, 9:48 PM
craig.topper added a child revision: D119518: [RISCV] Insert VSETVLI at the end of a basic block if we didn't produce BlockInfo.Exit..
frasercrmck accepted this revision.Feb 11 2022, 8:22 AM

Happy to revert but I don't know how that works with phab. As long as it's a proper revert when it's landed, I'm good.

This revision is now accepted and ready to land.Feb 11 2022, 8:22 AM
This revision was landed with ongoing or failed builds.Feb 11 2022, 9:34 AM
Closed by commit rGf35ac872b822: Revert "[RISCV] Fix a vsetvli insertion bug involving loads/stores." and "… (authored by craig.topper). · Explain Why
This revision was automatically updated to reflect the committed changes.
craig.topper added a commit: rGf35ac872b822: Revert "[RISCV] Fix a vsetvli insertion bug involving loads/stores." and "….

Revision Contents

PathSize
llvm/
lib/
Target/
RISCV/
21 lines
test/
CodeGen/
RISCV/
rvv/
6 lines

Diff 407922

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

Show First 20 Lines • Show All 993 Lines • ▼ Show 20 Linesbool RISCVInsertVSETVLI::needVSETVLIPHI(const VSETVLIInfo &Require,
// If all the incoming values to the PHI checked out, we don't need // If all the incoming values to the PHI checked out, we don't need
// to insert a VSETVLI. // to insert a VSETVLI.
return false; return false;
} }
void RISCVInsertVSETVLI::emitVSETVLIs(MachineBasicBlock &MBB) { void RISCVInsertVSETVLI::emitVSETVLIs(MachineBasicBlock &MBB) {
VSETVLIInfo CurInfo; VSETVLIInfo CurInfo;
// BBLocalInfo tracks the VL/VTYPE state the same way BBInfo.Change was
// calculated in computeIncomingVLVTYPE. We need this to apply
// canSkipVSETVLIForLoadStore the same way computeIncomingVLVTYPE did. We
// can't include predecessor information in that decision to avoid disagreeing
// with the global analysis.
VSETVLIInfo BBLocalInfo;
// Only be set if current VSETVLIInfo is from an explicit VSET(I)VLI. // Only be set if current VSETVLIInfo is from an explicit VSET(I)VLI.
MachineInstr *PrevVSETVLIMI = nullptr; MachineInstr *PrevVSETVLIMI = nullptr;
for (MachineInstr &MI : MBB) { for (MachineInstr &MI : MBB) {
// If this is an explicit VSETVLI or VSETIVLI, update our state. // If this is an explicit VSETVLI or VSETIVLI, update our state.
if (MI.getOpcode() == RISCV::PseudoVSETVLI || if (MI.getOpcode() == RISCV::PseudoVSETVLI ||
MI.getOpcode() == RISCV::PseudoVSETVLIX0 || MI.getOpcode() == RISCV::PseudoVSETVLIX0 ||
MI.getOpcode() == RISCV::PseudoVSETIVLI) { MI.getOpcode() == RISCV::PseudoVSETIVLI) {
// Conservatively, mark the VL and VTYPE as live. // Conservatively, mark the VL and VTYPE as live.
assert(MI.getOperand(3).getReg() == RISCV::VL && assert(MI.getOperand(3).getReg() == RISCV::VL &&
MI.getOperand(4).getReg() == RISCV::VTYPE && MI.getOperand(4).getReg() == RISCV::VTYPE &&
"Unexpected operands where VL and VTYPE should be"); "Unexpected operands where VL and VTYPE should be");
MI.getOperand(3).setIsDead(false); MI.getOperand(3).setIsDead(false);
MI.getOperand(4).setIsDead(false); MI.getOperand(4).setIsDead(false);
CurInfo = getInfoForVSETVLI(MI); CurInfo = getInfoForVSETVLI(MI);
BBLocalInfo = getInfoForVSETVLI(MI);
PrevVSETVLIMI = &MI; PrevVSETVLIMI = &MI;
continue; continue;
} }
uint64_t TSFlags = MI.getDesc().TSFlags; uint64_t TSFlags = MI.getDesc().TSFlags;
if (RISCVII::hasSEWOp(TSFlags)) { if (RISCVII::hasSEWOp(TSFlags)) {
VSETVLIInfo NewInfo = computeInfoForInstr(MI, TSFlags, MRI); VSETVLIInfo NewInfo = computeInfoForInstr(MI, TSFlags, MRI);
if (RISCVII::hasVLOp(TSFlags)) { if (RISCVII::hasVLOp(TSFlags)) {
Show All 13 Lines if (RISCVII::hasSEWOp(TSFlags)) {
MI.addOperand(MachineOperand::CreateReg(RISCV::VTYPE, /*isDef*/ false, MI.addOperand(MachineOperand::CreateReg(RISCV::VTYPE, /*isDef*/ false,
/*isImp*/ true)); /*isImp*/ true));
if (!CurInfo.isValid()) { if (!CurInfo.isValid()) {
// We haven't found any vector instructions or VL/VTYPE changes yet, // We haven't found any vector instructions or VL/VTYPE changes yet,
// use the predecessor information. // use the predecessor information.
assert(BlockInfo[MBB.getNumber()].Pred.isValid() && assert(BlockInfo[MBB.getNumber()].Pred.isValid() &&
"Expected a valid predecessor state."); "Expected a valid predecessor state.");
// Don't use predecessor information if there was an earlier instruction if (needVSETVLI(NewInfo, BlockInfo[MBB.getNumber()].Pred) &&
// in this block that allowed a vsetvli to be skipped for load/store.
if (!(BBLocalInfo.isValid() &&
canSkipVSETVLIForLoadStore(MI, NewInfo, BBLocalInfo)) &&
needVSETVLI(NewInfo, BlockInfo[MBB.getNumber()].Pred) &&
needVSETVLIPHI(NewInfo, MBB)) { needVSETVLIPHI(NewInfo, MBB)) {
insertVSETVLI(MBB, MI, NewInfo, BlockInfo[MBB.getNumber()].Pred); insertVSETVLI(MBB, MI, NewInfo, BlockInfo[MBB.getNumber()].Pred);
CurInfo = NewInfo; CurInfo = NewInfo;
BBLocalInfo = NewInfo;
} }
// We must update BBLocalInfo for every vector instruction.
if (!BBLocalInfo.isValid())
BBLocalInfo = NewInfo;
} else { } else {
assert(BBLocalInfo.isValid());
// If this instruction isn't compatible with the previous VL/VTYPE // If this instruction isn't compatible with the previous VL/VTYPE
// we need to insert a VSETVLI. // we need to insert a VSETVLI.
// If this is a unit-stride or strided load/store, we may be able to use // If this is a unit-stride or strided load/store, we may be able to use
// the EMUL=(EEW/SEW)*LMUL relationship to avoid changing vtype. // the EMUL=(EEW/SEW)*LMUL relationship to avoid changing vtype.
// NOTE: We can't use predecessor information for the store. We must // NOTE: We can't use predecessor information for the store. We must
// treat it the same as the first phase so that we produce the correct // treat it the same as the first phase so that we produce the correct
// vl/vtype for succesor blocks. // vl/vtype for succesor blocks.
if (!canSkipVSETVLIForLoadStore(MI, NewInfo, CurInfo) && if (!canSkipVSETVLIForLoadStore(MI, NewInfo, CurInfo) &&
Show All 19 Lines if (RISCVII::hasSEWOp(TSFlags)) {
NewInfo.hasSameVLMAX(CurInfo)) { NewInfo.hasSameVLMAX(CurInfo)) {
PrevVSETVLIMI->getOperand(2).setImm(NewInfo.encodeVTYPE()); PrevVSETVLIMI->getOperand(2).setImm(NewInfo.encodeVTYPE());
NeedInsertVSETVLI = false; NeedInsertVSETVLI = false;
} }
} }
if (NeedInsertVSETVLI) if (NeedInsertVSETVLI)
insertVSETVLI(MBB, MI, NewInfo, CurInfo); insertVSETVLI(MBB, MI, NewInfo, CurInfo);
CurInfo = NewInfo; CurInfo = NewInfo;
BBLocalInfo = NewInfo;
} }
} }
PrevVSETVLIMI = nullptr; PrevVSETVLIMI = nullptr;
} }
// If this is something updates VL/VTYPE that we don't know about, set // If this is something updates VL/VTYPE that we don't know about, set
// the state to unknown. // the state to unknown.
if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) || if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) ||
MI.modifiesRegister(RISCV::VTYPE)) { MI.modifiesRegister(RISCV::VTYPE)) {
CurInfo = VSETVLIInfo::getUnknown(); CurInfo = VSETVLIInfo::getUnknown();
BBLocalInfo = VSETVLIInfo::getUnknown();
PrevVSETVLIMI = nullptr; PrevVSETVLIMI = nullptr;
} }
} }
} }
bool RISCVInsertVSETVLI::runOnMachineFunction(MachineFunction &MF) { bool RISCVInsertVSETVLI::runOnMachineFunction(MachineFunction &MF) {
// Skip if the vector extension is not enabled. // Skip if the vector extension is not enabled.
const RISCVSubtarget &ST = MF.getSubtarget<RISCVSubtarget>(); const RISCVSubtarget &ST = MF.getSubtarget<RISCVSubtarget>();
▲ Show 20 Lines • Show All 47 Lines • Show Last 20 Lines

llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.mir

Show First 20 Lines • Show All 586 Lines • ▼ Show 20 Linesbody: |
; CHECK-NEXT: {{ $}} ; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1: ; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000) ; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: {{ $}} ; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[PHI:%[0-9]+]]:gpr = PHI [[COPY2]], %bb.0, %10, %bb.1 ; CHECK-NEXT: [[PHI:%[0-9]+]]:gpr = PHI [[COPY2]], %bb.0, %10, %bb.1
; CHECK-NEXT: [[PseudoVADD_VX_M1_:%[0-9]+]]:vr = PseudoVADD_VX_M1 [[PseudoVID_V_M1_]], [[PHI]], -1, 6, implicit $vl, implicit $vtype ; CHECK-NEXT: [[PseudoVADD_VX_M1_:%[0-9]+]]:vr = PseudoVADD_VX_M1 [[PseudoVID_V_M1_]], [[PHI]], -1, 6, implicit $vl, implicit $vtype
; CHECK-NEXT: [[MUL:%[0-9]+]]:gpr = MUL [[PHI]], [[SRLI]] ; CHECK-NEXT: [[MUL:%[0-9]+]]:gpr = MUL [[PHI]], [[SRLI]]
; CHECK-NEXT: [[ADD:%[0-9]+]]:gpr = ADD [[COPY]], [[MUL]] ; CHECK-NEXT: [[ADD:%[0-9]+]]:gpr = ADD [[COPY]], [[MUL]]
; FIXME: We insert a SEW=32,LMUL=1/2 VSETVLI here but no SEW=64,LMUL=1
; VSETVLI before the VADD above. This misconfigures the VADD in the case that
; the loop takes its backedge.
; CHECK-NEXT: dead $x0 = PseudoVSETVLIX0 killed $x0, 87, implicit-def $vl, implicit-def $vtype, implicit $vl
; CHECK-NEXT: PseudoVSE32_V_MF2 killed [[PseudoVADD_VX_M1_]], killed [[ADD]], -1, 5, implicit $vl, implicit $vtype ; CHECK-NEXT: PseudoVSE32_V_MF2 killed [[PseudoVADD_VX_M1_]], killed [[ADD]], -1, 5, implicit $vl, implicit $vtype
; CHECK-NEXT: [[ADDI:%[0-9]+]]:gpr = ADDI [[PHI]], 1 ; CHECK-NEXT: [[ADDI:%[0-9]+]]:gpr = ADDI [[PHI]], 1
; CHECK-NEXT: BLTU [[ADDI]], [[COPY1]], %bb.1 ; CHECK-NEXT: BLTU [[ADDI]], [[COPY1]], %bb.1
; CHECK-NEXT: PseudoBR %bb.2 ; CHECK-NEXT: PseudoBR %bb.2
; CHECK-NEXT: {{ $}} ; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2: ; CHECK-NEXT: bb.2:
; CHECK-NEXT: PseudoRET ; CHECK-NEXT: PseudoRET
bb.0: bb.0:
▲ Show 20 Lines • Show All 87 Lines • ▼ Show 20 Linesbody: |
; CHECK-NEXT: PseudoBR %bb.2 ; CHECK-NEXT: PseudoBR %bb.2
; CHECK-NEXT: {{ $}} ; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2.middle.block: ; CHECK-NEXT: bb.2.middle.block:
; CHECK-NEXT: [[COPY5:%[0-9]+]]:gpr = COPY $x0 ; CHECK-NEXT: [[COPY5:%[0-9]+]]:gpr = COPY $x0
; CHECK-NEXT: [[DEF:%[0-9]+]]:vr = IMPLICIT_DEF ; CHECK-NEXT: [[DEF:%[0-9]+]]:vr = IMPLICIT_DEF
; CHECK-NEXT: [[PseudoVMV_S_X_M1_:%[0-9]+]]:vr = PseudoVMV_S_X_M1 [[DEF]], [[COPY5]], 1, 5, implicit $vl, implicit $vtype ; CHECK-NEXT: [[PseudoVMV_S_X_M1_:%[0-9]+]]:vr = PseudoVMV_S_X_M1 [[DEF]], [[COPY5]], 1, 5, implicit $vl, implicit $vtype
; CHECK-NEXT: [[DEF1:%[0-9]+]]:vr = IMPLICIT_DEF ; CHECK-NEXT: [[DEF1:%[0-9]+]]:vr = IMPLICIT_DEF
; CHECK-NEXT: [[PseudoVREDSUM_VS_M1_:%[0-9]+]]:vr = PseudoVREDSUM_VS_M1 [[DEF1]], [[PseudoVADD_VV_M1_]], killed [[PseudoVMV_S_X_M1_]], 4, 5, implicit $vl, implicit $vtype ; CHECK-NEXT: [[PseudoVREDSUM_VS_M1_:%[0-9]+]]:vr = PseudoVREDSUM_VS_M1 [[DEF1]], [[PseudoVADD_VV_M1_]], killed [[PseudoVMV_S_X_M1_]], 4, 5, implicit $vl, implicit $vtype
; FIXME: There should be a VSETVLI here. ; CHECK-NEXT: dead $x0 = PseudoVSETIVLI 1, 80, implicit-def $vl, implicit-def $vtype
; CHECK-NEXT: PseudoVSE32_V_M1 killed [[PseudoVREDSUM_VS_M1_]], [[COPY]], 1, 5, implicit $vl, implicit $vtype :: (store (s32) into %ir.res) ; CHECK-NEXT: PseudoVSE32_V_M1 killed [[PseudoVREDSUM_VS_M1_]], [[COPY]], 1, 5, implicit $vl, implicit $vtype :: (store (s32) into %ir.res)
; CHECK-NEXT: PseudoRET ; CHECK-NEXT: PseudoRET
bb.0.entry: bb.0.entry:
liveins: $x10, $x12 liveins: $x10, $x12
%8:gpr = COPY $x12 %8:gpr = COPY $x12
%6:gpr = COPY $x10 %6:gpr = COPY $x10
%11:vr = PseudoVMV_V_I_M1 0, 4, 5 %11:vr = PseudoVMV_V_I_M1 0, 4, 5
Show All 29 Lines