Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Don't look for sext in RISCVCodeGenPrepare::visitAnd. #78798

Closed
wants to merge 2 commits into from

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jan 19, 2024

We want to know the upper 33 bits of the And Input are zero. SExt
only guarantees they are the same.

We originally checked for SExt or ZExt when we were using
isImpliedByDomCondition because a ZExt may have been changed to SExt
before we visited the And.

We are no longer using isImpliedByDomCondition so we can only look
for zext with the nneg flag.

While here, switch to PatternMatch to simplify the code.

Fixes #78783

We want to know the upper 33 bits of the And Input are zero. SExt
only guarantees they are the same.

We originally checked for SExt or ZExt when we were using
isImpliedByDomCondition because a ZExt may have been changed to SExt
before we visited the And.

We are no longer using isImpliedByDomCondition so we can only look
for zext with the nneg flag.

While here, switch to PatternMatch to simplify the code.

Fixes llvm#78783
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 19, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

We want to know the upper 33 bits of the And Input are zero. SExt
only guarantees they are the same.

We originally checked for SExt or ZExt when we were using
isImpliedByDomCondition because a ZExt may have been changed to SExt
before we visited the And.

We are no longer using isImpliedByDomCondition so we can only look
for zext with the nneg flag.

While here, switch to PatternMatch to simplify the code.

Fixes #78783


Full diff: https://github.com/llvm/llvm-project/pull/78798.diff

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp (+6-12)
  • (modified) llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll (+12)
diff --git a/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp b/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp
index 6434532afd40981..53fcc527e615dd4 100644
--- a/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp
+++ b/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp
@@ -21,6 +21,7 @@
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/Intrinsics.h"
+#include "llvm/IR/PatternMatch.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 
@@ -67,20 +68,13 @@ bool RISCVCodeGenPrepare::visitAnd(BinaryOperator &BO) {
   if (!BO.getType()->isIntegerTy(64))
     return false;
 
-  auto canBeSignExtend = [](Instruction *I) {
-    if (isa<SExtInst>(I))
-      return true;
-    if (isa<ZExtInst>(I))
-      return I->hasNonNeg();
-    return false;
-  };
+  using namespace PatternMatch;
 
-  // Left hand side should be a sext or zext nneg.
-  Instruction *LHS = dyn_cast<Instruction>(BO.getOperand(0));
-  if (!LHS || !canBeSignExtend(LHS))
+  // Left hand side should be a zext nneg.
+  Value *LHSSrc;
+  if (!match(BO.getOperand(0), m_NNegZExt(m_Value(LHSSrc))))
     return false;
 
-  Value *LHSSrc = LHS->getOperand(0);
   if (!LHSSrc->getType()->isIntegerTy(32))
     return false;
 
@@ -100,7 +94,7 @@ bool RISCVCodeGenPrepare::visitAnd(BinaryOperator &BO) {
 
   // Sign extend the constant and replace the And operand.
   C = SignExtend64<32>(C);
-  BO.setOperand(1, ConstantInt::get(LHS->getType(), C));
+  BO.setOperand(1, ConstantInt::get(RHS->getType(), C));
 
   return true;
 }
diff --git a/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll b/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll
index 0e4e04af4a3fe7e..2179a0d26cf9826 100644
--- a/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll
+++ b/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll
@@ -91,3 +91,15 @@ for.body:                                         ; preds = %for.body, %for.body
   %niter.ncmp.1 = icmp eq i64 %niter.next.1, %unroll_iter
   br i1 %niter.ncmp.1, label %for.cond.cleanup.loopexit.unr-lcssa, label %for.body
 }
+
+; Make sure we do not change 4294967295 to -1 here.
+define i64 @bug(i32 %x) {
+; CHECK-LABEL: @bug(
+; CHECK-NEXT:    [[A:%.*]] = sext i32 [[X:%.*]] to i64
+; CHECK-NEXT:    [[B:%.*]] = and i64 [[A]], 4294967295
+; CHECK-NEXT:    ret i64 [[B]]
+;
+  %a = sext i32 %x to i64
+  %b = and i64 %a, 4294967295
+  ret i64 %b
+}

@nikic
Copy link
Contributor

nikic commented Jan 19, 2024

(An alternative would be to check that the constant isInt<31> I think. But your approach seems preferable.)

@topperc
Copy link
Collaborator Author

topperc commented Jan 19, 2024

Committed as 66cea71 and 9396891

@topperc topperc closed this Jan 19, 2024
@topperc topperc deleted the pr/78783 branch January 19, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RISCV64 miscompile at -O1
3 participants