[llvm] db6bee5 - [RISCV] Fix miscompile in SExtWRemoval due to early return ignoring other sources

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2023-02-08T11:45:48-08:00
New Revision: db6bee5fec0d7fdfc18005c5c5ccd15f1ede945d

URL: https://github.com/llvm/llvm-project/commit/db6bee5fec0d7fdfc18005c5c5ccd15f1ede945d
DIFF: https://github.com/llvm/llvm-project/commit/db6bee5fec0d7fdfc18005c5c5ccd15f1ede945d.diff

LOG: [RISCV] Fix miscompile in SExtWRemoval due to early return ignoring other sources

This code is walking back through a worklist of sources. All of the sources need to be sign extending for the result to be true. We had a case which returned rather than continued, which causes a miscompile when another source was not sign extended. The flawed logic was introduced in Dec 22, by change 844430bcc377.

This was recently exposed in a stage2 build of llvm-tablegen when we switched from using llvm::Optional to std::optional. The stars aligned in just the wrong way, and we started actively miscompiling idiomatic optional usage. std::optional<uint32_t> appears to use the top 32 bits of the word on RV64 for its tag.

Differential Revision: https://reviews.llvm.org/D143594

Added: 
    

Modified: 
    llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
    llvm/test/CodeGen/RISCV/sextw-removal.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp b/llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
index 2ee228d728258..a26a3f2411f8a 100644
--- a/llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
+++ b/llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
@@ -175,8 +175,9 @@ static bool isSignExtendedW(Register SrcReg, const MachineRegisterInfo &MRI,
 
         const AttributeSet &Attrs = CalleeFn->getAttributes().getRetAttrs();
         unsigned BitWidth = IntTy->getBitWidth();
-        return (BitWidth <= 32 && Attrs.hasAttribute(Attribute::SExt)) ||
-               (BitWidth < 32 && Attrs.hasAttribute(Attribute::ZExt));
+        if ((BitWidth <= 32 && Attrs.hasAttribute(Attribute::SExt)) ||
+            (BitWidth < 32 && Attrs.hasAttribute(Attribute::ZExt)))
+          continue;
       }
 
       if (!AddRegDefToWorkList(CopySrcReg))

diff  --git a/llvm/test/CodeGen/RISCV/sextw-removal.ll b/llvm/test/CodeGen/RISCV/sextw-removal.ll
index 6d6e2950b02aa..ffe64db623508 100644
--- a/llvm/test/CodeGen/RISCV/sextw-removal.ll
+++ b/llvm/test/CodeGen/RISCV/sextw-removal.ll
@@ -1376,8 +1376,6 @@ define signext i32 @sextw_sh2add(i1 zeroext %0, ptr %1, i32 signext %2, i32 sign
 }
 
 ; Negative test - an explicit sext.w *is* required
-; FIXME: This is currently demonstrating an active miscompile as the high
-; bits of s0 are *not* the sign extended zero of bit 32 on the untaken path.
 define signext i32 @test19(i64 %arg, i1 zeroext %c1, i1 zeroext %c2, ptr %p) nounwind {
 ; CHECK-LABEL: test19:
 ; CHECK:       # %bb.0: # %bb
@@ -1397,7 +1395,7 @@ define signext i32 @test19(i64 %arg, i1 zeroext %c1, i1 zeroext %c2, ptr %p) nou
 ; CHECK-NEXT:    mv s0, a0
 ; CHECK-NEXT:  .LBB23_2: # %bb7
 ; CHECK-NEXT:    call side_effect at plt
-; CHECK-NEXT:    mv a0, s0
+; CHECK-NEXT:    sext.w a0, s0
 ; CHECK-NEXT:    ld ra, 8(sp) # 8-byte Folded Reload
 ; CHECK-NEXT:    ld s0, 0(sp) # 8-byte Folded Reload
 ; CHECK-NEXT:    addi sp, sp, 16