From 6eb8c35d845d2e4698df9ec4ecb103933b018087 Mon Sep 17 00:00:00 2001 From: Philip Reames Date: Fri, 16 Jun 2023 16:47:39 -0700 Subject: [PATCH] [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 --- llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp index 3732bf2..960e6cb 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 -- 2.7.4