[PATCH] D88276: [IsKnownNonZero] Handle the case with non-constant phi nodes

Sun Sep 27 22:42:41 PDT 2020

jdoerfert added a comment.

@nikic thanks for catching this, these are a pain to debug later on (I remember D69571 <https://reviews.llvm.org/D69571>). That said, we should probably add a test that shows why this is necessary. Can you take a look at the test in D71181 <https://reviews.llvm.org/D71181> and https://reviews.llvm.org/D60846#1497243 to make sure we have a similar one for coverage here?

> Please also note that right before phi handling we have a switch handling. It does not use one level depth as well as it does not modify the context instruction while select is actually can be considered as equivalent of phi node. Should be switch fixed as well?

Select do not require a change in context (as far as I can tell). A PHI is "assigned/evaluated" on the incoming edge, not in the PHI block. The key difference is that the incoming value does not dominate the phi while this has to be the case for the select.

---

In D88276#2297244 <https://reviews.llvm.org/D88276#2297244>, @skatkov wrote:

> please take a look. Still think that restriction to one level is too strong but let's start with it.

(Disclaimer, I'm obviously biased towards the Attributor.)
TBH, I don't think such logic belongs here anyway. The problem is that we do not "cache" results but simply recompute them, given use-def cycles that can spiral out of control.
If you run the Attributor it will deduce and annotate various locations with nonnull, if we are missing some we can talk about ways to improve it.

---

Nit: typo in the commit message.
Nit: consider `Q.getWithInstruction`, no strong feelings.

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

https://reviews.llvm.org/D88276