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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:310
+        continue;
+      if (auto *CI = dyn_cast_or_null<ConstantInt>(*C)) {
+        if (CI->isZero())
----------------
jdoerfert wrote:
> baziotis wrote:
> > jdoerfert wrote:
> > > 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
> > I get it from a dataflow framework perspective. I also think `Optional` is a good choice for a best/worse value (infinites etc.). It's just that it makes it tricky for me if a function returns an optional pointer, which pointer can be `null`. Because the optional family of structures are a lot of times used exactly to avoid a pointer acting both as a flag and an address. But that's just my perspective and it's probably not a particularly useful review to you :)
> But `null` means something. All values have a dedicated and important meaning. We "cannot" just not use an optional, nor is the optional used to avoid pointers or null.
> 
> ```
>    None -> Nothing yet (best)
>     |
>   Undef -> Second best
> /   |  \ 
> ... 0   1  ...   -> values/constants or similar
> \   |   /
>   nullptr       -> worst
> ```
> 
> Does this make more sense?
I don't have a misunderstanding on what `None` and `null` mean in this context (although I had  initially...), but thank you. The problem is that you are using `Optional` //and// `null` in a way that it is not usually used. 

Let me give a different and more simplified example to make my case clear. Let's say that you have a function which returns a number. But, you also want to support the concept of "infinite" in which case you return `None`. So, it looks something like this:
```
Optional<int> Number = getNumber(...);
if (!Number.hasValue())
  // Do something that has to do with infinite
else
  // .getValue() and handle int
```

So, what's the problem IMHO with this code ? `Optional` is //usually// used to denote "you didn't get a value", "that call failed" etc. It's usually used in a pattern to denote some kind of "failure". But in //my// pattern, it is actually used as just a different meaningful value. For this reason, if the code does not make an effort to make clear that I'm following a different pattern than the common one, it creates a congestion to the mind of the reader.

In the case of `getAssumedConstant()`, when I first read it, unconsciously I assumed `None` means "you didn't get a constant". And because of that, it followed `nullptr` had no place because its usual place is to denote failure but `Optional` was used for that. It turned that all these assumptions were wrong because I was thinking in terms of the common case. Now, granted, I should've thought that through. But the usage of common values for different uses than the common ones can be tricky. I hope this provided some value for you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103861