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

nuw nsw not deduced for add 1 inbounds of range-restricted length #87854

Closed
scottmcm opened this issue Apr 6, 2024 · 3 comments
Closed

nuw nsw not deduced for add 1 inbounds of range-restricted length #87854

scottmcm opened this issue Apr 6, 2024 · 3 comments

Comments

@scottmcm
Copy link

scottmcm commented Apr 6, 2024

(Context: I tried adding assumes to slice lengths in rust -- rust-lang/rust#122926 -- and was surprised that certain things didn't optimize away that I would have expected to. The example here is basically if i < slice.len() { do_something_with(i + 1); }).)

I tried the following: https://llvm.godbolt.org/z/qM5bnMxxj

define noundef i64 @src(i64 noundef %x.1, i64 noundef %i) unnamed_addr #0 {
start:
  %0 = icmp sge i64 %x.1, 0
  tail call void @llvm.assume(i1 %0)
  %inbounds = icmp ult i64 %i, %x.1
  br i1 %inbounds, label %do_something, label %done

do_something:
  %next = add i64 %i, 1  ;  <-- look here
  br label %done

done:
  %r = phi i64 [ %next, %do_something ], [-1, %start ]
  ret i64 %r
}

expecting to see the add become add nuw nsw because of the range restriction on %x.1, but it doesn't.

Alive proof that it would be ok to do so here: https://alive2.llvm.org/ce/z/3jHXNu

@dtcxzyw
Copy link
Member

dtcxzyw commented Apr 7, 2024

It has been supported by CVP: https://godbolt.org/z/ezdrzn4Yv.

Did you mean: https://alive2.llvm.org/ce/z/xNoymj

define i32 @src(i32 %x.1, i32 noundef %i) {
  %cond = icmp sgt i32 %x.1, -1
  tail call void @llvm.assume(i1 %cond)
  %inbounds = icmp ult i32 %i, %x.1
  %next = add i32 %i, 1
  %spec.select = select i1 %inbounds, i32 %next, i32 -1
  ret i32 %spec.select
}

define i32 @tgt(i32 %x.1, i32 noundef %i) {
  %cond = icmp sgt i32 %x.1, -1
  tail call void @llvm.assume(i1 %cond)
  %inbounds = icmp ult i32 %i, %x.1
  %next = add nuw nsw i32 %i, 1
  %spec.select = select i1 %inbounds, i32 %next, i32 -1
  ret i32 %spec.select
}

@scottmcm
Copy link
Author

scottmcm commented Apr 7, 2024

Did you mean

I guess so? I don't know enough about phase ordering to say confidently whether CVP can actually handle the scenario I was hitting, just that running the normal O3 passes on the provided source IR doesn't give me an output with add nuw nsw.

dtcxzyw added a commit that referenced this issue Apr 13, 2024
This patch uses `getConstantRangeAtUse` to infer nsw/nuw flags with
at-use info. It will enables more optimizations in InstCombine.

Compile-time impact:
http://llvm-compile-time-tracker.com/compare.php?from=a5ed14bc8e122fa5ac0aa81f8d8390931bd6b4e4&to=a83d3402b663439b91cb37a046fb7ac0220ba5c7&stat=instructions%3Au

Related issue: #87854
bazuzi pushed a commit to bazuzi/llvm-project that referenced this issue Apr 15, 2024
This patch uses `getConstantRangeAtUse` to infer nsw/nuw flags with
at-use info. It will enables more optimizations in InstCombine.

Compile-time impact:
http://llvm-compile-time-tracker.com/compare.php?from=a5ed14bc8e122fa5ac0aa81f8d8390931bd6b4e4&to=a83d3402b663439b91cb37a046fb7ac0220ba5c7&stat=instructions%3Au

Related issue: llvm#87854
@dtcxzyw
Copy link
Member

dtcxzyw commented Jul 27, 2024

Closed as there is little benefit to use block value in CVP.

diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp b/llvm/lib/Analysis/LazyValueInfo.cpp
index 4c023ed5ed8f..a35d250608ea 100644
--- a/llvm/lib/Analysis/LazyValueInfo.cpp
+++ b/llvm/lib/Analysis/LazyValueInfo.cpp
@@ -1531,7 +1531,7 @@ ValueLatticeElement LazyValueInfoImpl::getValueInBlock(Value *V, BasicBlock *BB,
   LLVM_DEBUG(dbgs() << "LVI Getting block end value " << *V << " at '"
                     << BB->getName() << "'\n");
 
-  assert(BlockValueStack.empty() && BlockValueSet.empty());
+  // assert(BlockValueStack.empty() && BlockValueSet.empty());
   std::optional<ValueLatticeElement> OptResult = getBlockValue(V, BB, CxtI);
   if (!OptResult) {
     solve();
@@ -1601,12 +1601,12 @@ ValueLatticeElement LazyValueInfoImpl::getValueAtUse(const Use &U) {
         break;
       if (CurrU->getOperandNo() == 1)
         CondVal =
-            *getValueFromCondition(V, SI->getCondition(), /*IsTrueDest*/ true,
-                                   /*UseBlockValue*/ false);
+            getValueFromCondition(V, SI->getCondition(), /*IsTrueDest=*/true,
+                                   /*UseBlockValue=*/true);
       else if (CurrU->getOperandNo() == 2)
         CondVal =
-            *getValueFromCondition(V, SI->getCondition(), /*IsTrueDest*/ false,
-                                   /*UseBlockValue*/ false);
+            getValueFromCondition(V, SI->getCondition(), /*IsTrueDest=*/false,
+                                   /*UseBlockValue=*/true);
     } else if (auto *PHI = dyn_cast<PHINode>(CurrI)) {
       // TODO: Use non-local query?
       CondVal = *getEdgeValueLocal(V, PHI->getIncomingBlock(*CurrU),

@dtcxzyw dtcxzyw closed this as completed Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants