[llvm] 6eb8c35 - [RISCV] Fix a latent miscompile in doPeepholeMaskedRVV

[Philip Reames via llvm-commits]

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