[llvm] 6eb8c35 - [RISCV] Fix a latent miscompile in doPeepholeMaskedRVV
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 16 16:52:48 PDT 2023
Author: Philip Reames
Date: 2023-06-16T16:47:39-07:00
New Revision: 6eb8c35d845d2e4698df9ec4ecb103933b018087
URL: https://github.com/llvm/llvm-project/commit/6eb8c35d845d2e4698df9ec4ecb103933b018087
DIFF: https://github.com/llvm/llvm-project/commit/6eb8c35d845d2e4698df9ec4ecb103933b018087.diff
LOG: [RISCV] Fix a latent miscompile in doPeepholeMaskedRVV
The code was using the tail policy being "agnostic" to select a instruction whose semantics were "undefined". This was almost always fine (as the pass through operand was usually implicit_def), but could in theory lead to a miscompile. I don't actually have a test case as it requires a later transform to exploit the wrong tail policy state, and I couldn't easily figure out to get vsetvli insertion to miscompile given the wrong state. This was spotted by inspection, and it may be a miscompile in theory only at the moment.
Note that this may cause regressions if there are instructions for which we either don't have a _TU pseudo form, or the _TU pseudo form is missing a policy operand. When I was first looking at this, I saw exactly that, and D153067 exists to add the missing policy operand I noticed.
As a later follow up, I want to always force the use of _TU, but it seemed good to fix the bug, then driven the _TU transition in a separate patch.
Differential Revision: https://reviews.llvm.org/D153070
Added:
Modified:
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index 3732bf28ac46f..960e6cbae3003 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -3160,6 +3160,11 @@ static bool usesAllOnesMask(SDNode *N, unsigned MaskOpIdx) {
IsVMSet(MaskSetter.getMachineOpcode());
}
+static bool isImplicitDef(SDValue V) {
+ return V.isMachineOpcode() &&
+ V.getMachineOpcode() == TargetOpcode::IMPLICIT_DEF;
+}
+
// Optimize masked RVV pseudo instructions with a known all-ones mask to their
// corresponding "unmasked" pseudo versions. The mask we're interested in will
// take the form of a V0 physical register operand, with a glued
@@ -3186,8 +3191,7 @@ bool RISCVDAGToDAGISel::doPeepholeMaskedRVV(SDNode *N) {
if (I->UnmaskedTUPseudo == I->UnmaskedPseudo) {
UseTUPseudo = true;
} else {
- if (!(N->getConstantOperandVal(*TailPolicyOpIdx) &
- RISCVII::TAIL_AGNOSTIC)) {
+ if (!isImplicitDef(N->getOperand(0))) {
// Keep the true-masked instruction when there is no unmasked TU
// instruction
if (I->UnmaskedTUPseudo == I->MaskedPseudo)
@@ -3232,11 +3236,6 @@ bool RISCVDAGToDAGISel::doPeepholeMaskedRVV(SDNode *N) {
return true;
}
-static bool isImplicitDef(SDValue V) {
- return V.isMachineOpcode() &&
- V.getMachineOpcode() == TargetOpcode::IMPLICIT_DEF;
-}
-
// Try to fold away VMERGE_VVM instructions. We handle these cases:
// -Masked TU VMERGE_VVM combined with an unmasked TA instruction instruction
// folds to a masked TU instruction. VMERGE_VVM must have have merge operand
More information about the llvm-commits
mailing list