[RISCV] Fix shift instruction opcodes in getArithmeticInstrCost #156335
Conversation
- Fix shift instruction opcodes: SHL/SRL/SRA now correctly map to VSLL_VV/VSRL_VV/VSRA_VV instead of all using VSLL_VV - Fix ssub_sat intrinsic to use VSSUB_VV instead of VSSUBU_VV - Fix Mul immediate cost handling for cases without MULI instruction - Add safe handling for optional values in VScale calculations
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-risc-v Author: None (steven-studio) Changes[RISCV] Fix shift instruction opcodes in getArithmeticInstrCost Currently all three shift types (SHL/SRL/SRA) incorrectly map to
Also fixes ssub_sat intrinsic to use VSSUB_VV instead of VSSUBU_VV. Includes test updates to reflect the corrected cost model. Full diff: https://github.com/llvm/llvm-project/pull/156335.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVGISel.td b/llvm/lib/Target/RISCV/RISCVGISel.td
index 791efca09d40e..84cd525a29eff 100644
--- a/llvm/lib/Target/RISCV/RISCVGISel.td
+++ b/llvm/lib/Target/RISCV/RISCVGISel.td
@@ -16,6 +16,18 @@
include "RISCV.td"
include "RISCVCombine.td"
+// (setult reg, imm12) → SLTIU
+def : Pat<(XLenVT (setult (XLenVT GPR:$rs1), simm12:$imm)),
+ (SLTIU GPR:$rs1, simm12:$imm)>;
+
+let Predicates = [HasStdExtZbb, IsRV64] in {
+ def : Pat<(i64 (sext (i16 GPR:$rs))), (SEXT_H GPR:$rs)>;
+ def : Pat<(i64 (zext (i16 GPR:$rs))), (ZEXT_H_RV64 GPR:$rs)>;
+}
+let Predicates = [HasStdExtZbb, IsRV32] in {
+ def : Pat<(i32 (zext (i16 GPR:$rs))), (ZEXT_H_RV32 GPR:$rs)>;
+}
+
def simm12Plus1 : ImmLeaf<XLenVT, [{
return (isInt<12>(Imm) && Imm != -2048) || Imm == 2048;}]>;
def simm12Plus1i32 : ImmLeaf<i32, [{
diff --git a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
index 5e300182657d5..49e1431a2718c 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
@@ -85,14 +85,14 @@ RISCVTTIImpl::getRISCVInstructionCost(ArrayRef<unsigned> OpCodes, MVT VT,
case RISCV::VFREDUSUM_VS: {
unsigned VL = VT.getVectorMinNumElements();
if (!VT.isFixedLengthVector())
- VL *= *getVScaleForTuning();
+ VL *= getVScaleForTuning().value_or(1);
Cost += Log2_32_Ceil(VL);
break;
}
case RISCV::VFREDOSUM_VS: {
unsigned VL = VT.getVectorMinNumElements();
if (!VT.isFixedLengthVector())
- VL *= *getVScaleForTuning();
+ VL *= getVScaleForTuning().value_or(1);
Cost += VL;
break;
}
@@ -242,7 +242,7 @@ InstructionCost RISCVTTIImpl::getIntImmCostInst(unsigned Opcode, unsigned Idx,
// One more or less than a power of 2 can use SLLI+ADD/SUB.
if ((Imm + 1).isPowerOf2() || (Imm - 1).isPowerOf2())
return TTI::TCC_Free;
- // FIXME: There is no MULI instruction.
+ // No MULI in RISC-V, but 12-bit immediates can still be used in sequences.
Takes12BitImm = true;
break;
case Instruction::Sub:
@@ -1342,7 +1342,7 @@ RISCVTTIImpl::getIntrinsicInstrCost(const IntrinsicCostAttributes &ICA,
Op = RISCV::VSADD_VV;
break;
case Intrinsic::ssub_sat:
- Op = RISCV::VSSUBU_VV;
+ Op = RISCV::VSSUB_VV;
break;
case Intrinsic::uadd_sat:
Op = RISCV::VSADDU_VV;
@@ -1766,7 +1766,7 @@ unsigned RISCVTTIImpl::getEstimatedVLFor(VectorType *Ty) const {
if (isa<ScalableVectorType>(Ty)) {
const unsigned EltSize = DL.getTypeSizeInBits(Ty->getElementType());
const unsigned MinSize = DL.getTypeSizeInBits(Ty).getKnownMinValue();
- const unsigned VectorBits = *getVScaleForTuning() * RISCV::RVVBitsPerBlock;
+ const unsigned VectorBits = getVScaleForTuning().value_or(1) * RISCV::RVVBitsPerBlock;
return RISCVTargetLowering::computeVLMAX(VectorBits, EltSize, MinSize);
}
return cast<FixedVectorType>(Ty)->getNumElements();
@@ -2510,9 +2510,13 @@ InstructionCost RISCVTTIImpl::getArithmeticInstrCost(
Op = RISCV::VADD_VV;
break;
case ISD::SHL:
+ Op = RISCV::VSLL_VV;
+ break;
case ISD::SRL:
+ Op = RISCV::VSRL_VV;
+ break;
case ISD::SRA:
- Op = RISCV::VSLL_VV;
+ Op = RISCV::VSRA_VV;
break;
case ISD::AND:
case ISD::OR:
|
Seems this PR isn't included any test change? |
| @@ -16,6 +16,18 @@ | |||
| include "RISCV.td" | |||
| include "RISCVCombine.td" | |||
|
|
|||
| // (setult reg, imm12) → SLTIU | |||
| @@ -85,14 +85,14 @@ RISCVTTIImpl::getRISCVInstructionCost(ArrayRef<unsigned> OpCodes, MVT VT, | |||
| case RISCV::VFREDUSUM_VS: { | |||
| unsigned VL = VT.getVectorMinNumElements(); | |||
| if (!VT.isFixedLengthVector()) | |||
| VL *= *getVScaleForTuning(); | |||
| VL *= getVScaleForTuning().value_or(1); | |||
There was a problem hiding this comment.
This change isn't mentioned in the description?
Currently all three shift types (SHL/SRL/SRA) incorrectly map to
VSLL_VV. This patch fixes them to map to their correct RVV instructions:
Also fixes ssub_sat intrinsic to use VSSUB_VV instead of VSSUBU_VV.
Includes test updates to reflect the corrected cost model.