[PATCH] D123748: [ValueTracking] Added support to deduce PHI Nodes values being a power of 2

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2173
+  if (const PHINode *PN = dyn_cast<PHINode>(V)) {
+    unsigned NewDepth = std::max(Depth, MaxAnalysisRecursionDepth - 1);
+    Query RecQ = Q;
----------------
Carrot wrote:
> Please add a comment here. I searched the source code for a while to understand its purpose.
This request was not answered. We are using max depth to avoid a compile-time explosion because we are recursing on every phi operand?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2181-2187
+    // Recursively check all incoming values
+    return llvm::all_of(PN->operands(), [&](const Use &U) {
+      if (U.get() == PN)
+        return true;
+      RecQ.CxtI = PN->getIncomingBlock(U)->getTerminator();
+      return isKnownToBeAPowerOfTwo(U.get(), OrZero, NewDepth, RecQ);
+    });
----------------
It would be safer to split this patch into 2 parts. The first would be just this part (and eliminate adding `isPowerOfTwoRecurrence` until the next patch). We can't currently handle even minimal non-loop patterns like:

```

define i64 @known_power_of_two_urem_phi(i64 %x, i1 %b) {
entry:
  br i1 %b, label %t, label %f

t:                
  br label %end

f:           
  br label %end

end:             
  %p = phi i64 [ 4096, %t ], [ 4, %f ]
  %r = urem i64 %x, %p
  ret i64 %r
}


```


================
Comment at: llvm/test/Analysis/ValueTracking/known-power-of-two-urem.ll:7
+
+define i64 @known_power_of_two_urem_select(i64 %size, i1 %cmp, i1 %cmp1) {
+; CHECK-LABEL: @known_power_of_two_urem_select(
----------------
If I understand correctly, there's no difference for this test - no phis, and it already works as expected.

Please pre-commit the tests with baseline CHECKs, so we just show test diffs in this review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123748/new/

https://reviews.llvm.org/D123748