[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:310
+        continue;
+      if (auto *CI = dyn_cast_or_null<ConstantInt>(*C)) {
+        if (CI->isZero())
----------------
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.


================
Comment at: llvm/test/Transforms/Attributor/lvi-for-ashr.ll:45
+; IS__TUNIT_NPM:       bb_then:
+; IS__TUNIT_NPM-NEXT:    [[DOT:%.*]] = select i1 true, i32 3, i32 2
+; IS__TUNIT_NPM-NEXT:    br label [[RETURN]]
----------------
baziotis wrote:
> Why is this not replaced by 3 now?
Because we never ask for the simplified value but only look through it in the `genericValueTraversal`. I'll improve AAPotentialValues in a follow up patch. I'm also considering to ask for a simplified value as part of the `genericValueTraversal` but I'll need to double check if that is the best idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103861