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

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: craig.topper, asb, kito-cheng, frasercrmck.
Herald added subscribers: luke, VincentWu, vkmr, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, niosHD, sabuasal, bollu, simoncook, johnrusso, rbar, hiraditya, arichardson, mcrosier.
Herald added a project: All.
reames requested review of this revision.
Herald added subscribers: pcwang-thead, eopXD, MaskRay.
Herald added a project: LLVM.

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 <https://reviews.llvm.org/rG844430bcc3778094ac0e0dd085062809b7f6d666>.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143594

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


Index: llvm/test/CodeGen/RISCV/sextw-removal.ll
===================================================================
--- llvm/test/CodeGen/RISCV/sextw-removal.ll
+++ llvm/test/CodeGen/RISCV/sextw-removal.ll
@@ -1376,8 +1376,6 @@
 }
 
 ; 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 @@
 ; 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
Index: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
===================================================================
--- llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
+++ llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp
@@ -175,8 +175,9 @@
 
         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))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D143594.495880.patch
Type: text/x-patch
Size: 1637 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230208/759ad57e/attachment.bin>