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

[reames]

Summary

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.

[craig.topper]

LGTM. We should backport to LLVM 16.

[asb]

LGTM. Agreed this should be backported to 16.0.

[reames]

In D143594#4113547, @craig.topper wrote:

LGTM. We should backport to LLVM 16.

Do we have our process for requesting a backport documented somewhere? Haven't done one before.

[asb]

In D143594#4113610, @reames wrote:

In D143594#4113547, @craig.topper wrote:

LGTM. We should backport to LLVM 16.

Do we have our process for requesting a backport documented somewhere? Haven't done one before.

It's now nice and mostly automated. Create an issue on GitHub for the cherry-pick, add it to the LLVM 16.0.0 release milestone, and you can then use these commands (in you case, /cherry-pick foo) in a comment or the initial issue description to trigger a bot to create the appropriate PR.

[reames]

https://github.com/llvm/llvm-project/issues/60608 for the backport. First time doing this, so we'll see how much I manage to screw this up. :)

llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp

case RISCV::COPY: {
           return false;
 
         auto *IntTy = dyn_cast<IntegerType>(CalleeFn->getReturnType());
         if (!IntTy)
           return false;
 
         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))
         return false;
 
       break;
     }
 

llvm/test/CodeGen/RISCV/sextw-removal.ll

8:                                                ; preds = %5
   br label %9
 
 9:                                                ; preds = %5, %8
   %10 = add i32 %7, %4
   ret i32 %10
 }
 
 ; 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
 ; CHECK-NEXT:    addi sp, sp, -16
 ; CHECK-NEXT:    sd ra, 8(sp) # 8-byte Folded Spill
 ; CHECK-NEXT:    sd s0, 0(sp) # 8-byte Folded Spill
 ; CHECK-NEXT:    neg a0, a1
 ; CHECK-NEXT:    li a1, 1
 ; CHECK-NEXT:    slli a1, a1, 32
 ; CHECK-NEXT:    addi s0, a1, 35
 ; CHECK-NEXT:    and s0, a0, s0
 ; CHECK-NEXT:    sd s0, 0(a3)
 ; CHECK-NEXT:    beqz a2, .LBB23_2
 ; CHECK-NEXT:  # %bb.1: # %bb2
 ; CHECK-NEXT:    li a0, 0
 ; CHECK-NEXT:    call bar@plt
 ; CHECK-NEXT:    mv s0, a0
 ; CHECK-NEXT:  .LBB23_2: # %bb7
 ; CHECK-NEXT:    call side_effect@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
 ; CHECK-NEXT:    ret
 ;
 ; NOREMOVAL-LABEL: test19:
 ; NOREMOVAL:       # %bb.0: # %bb
 ; NOREMOVAL-NEXT:    addi sp, sp, -16