[PATCH] D124889: [ValueTracking] Add support to deduce a PHI node being a power of 2 if each incoming value is a power of 2.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2140
+      RecQ.CxtI = PN->getIncomingBlock(U)->getTerminator();
+      return isKnownToBeAPowerOfTwo(U.get(), OrZero, Depth, RecQ);
+    });
----------------
As mentioned in the earlier patch (D123748), the Depth should be limited here to prevent a compile-time explosion. If the phi has a large number of operands, this could consume a lot of time - O(Operands^Depth).

The code that this patch is copying from was added with D88276, and there is more discussion about it there.
Note that the limit was derived from this code, but it dropped the explanatory code comment:
https://github.com/llvm/llvm-project/blob/d2166076b882e38becf3657ea830ffd2b6a5695e/llvm/lib/Analysis/ValueTracking.cpp#L1574-L1606

I'm not sure why it is using `std::max` instead of just passing `MaxAnalysisRecursionDepth - 1`. If I'm seeing the tests in this patch correctly, we will not see a functional difference on either of those tests by limiting the recursion.

The other non-obvious part of this patch is the setting of the context instruction; that is copied/derived from:
D71181

All of this copying and mutation suggests that we really need to create a helper function, so the logic is consistent for phi analysis across these functions. If that isn't feasible, at least copy the code comments, so we don't have to dig all over the place to figure out why this code does what it does. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124889