[PATCH] D103861: [Attributor] Look through selects in genericValueTraversal

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:309
+      if (!C.hasValue() || isa_and_nonnull<UndefValue>(*C))
+        continue;
+      if (auto *CI = dyn_cast_or_null<ConstantInt>(*C)) {
----------------
baziotis wrote:
> kuter wrote:
> > `A.getAssumedConstant` returns None if the value simplification wasn't possible. 
> > If the value is simplified but not constant it returns nullptr.
> > 
> > Shouldn't we visit the true and false options when there is no simplification ? 
> > I don't understand why we `continue` here. I think callback  would receive the select instruction itself.
> > Which is very different from previous behavior.
> Well, it's an intricate design but now it's clear, thanks!
> A.getAssumedConstant returns None if the value simplification wasn't possible. 

No. None means "the value is dead/undetermined so far". You can basically say "there is no value yet".
Eventually, a value other than none is returned, *iff* the value is in fact live. 


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:310
+        continue;
+      if (auto *CI = dyn_cast_or_null<ConstantInt>(*C)) {
+        if (CI->isZero())
----------------
baziotis wrote:
> jdoerfert wrote:
> > baziotis wrote:
> > > Not important but `SI->getCondition()` should return either an `i1` or a vector of `i1`. I don't know what's happening in `getAssumedConstant()` but I assume it doesn't deal with vectors which means that if `C.hasValue()` and it is //not// `undef`, then it //must// be a `ConstantInt`? So, maybe put that in an `assert()`. OTOH, if later you decide to deal with vectors, those asserts will fire while in this you can just add an `else if`. Anyway, just wanted to bring it to the foreground and you can choose what is better.
> > It must not be a constantint, even if there are no vectors. 
> > 
> > ```
> > int foo(bool A) {
> >   B = !A;
> >   C = !B;
> >   return C ? 0: 42;
> > }
> > ```
> > `C` here is a i1, simplified value is (potentially) `A` which is neither constant nor vector.
> Ok, something I probably missed here, which I should've have inferred from the usage of `isa_and_nonnull` and `dyn_cast_or_null`, is that `C` may have a value (i.e., `C.hasValue() == true`) but its value may be `null`. Hm.., then why the `Optional`? 
We use Optional because our lattice needs a best value (bottom in these slides https://engineering.purdue.edu/~milind/ece468/2015fall/lecture-10.5-6up.pdf), see https://reviews.llvm.org/rGe93ac1e2de66e8feae3cec3b6c0707b14c79dfeb


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103861