[PATCH] D66967: [Attributor] ValueSimplify Abstract Attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/test/Transforms/FunctionAttrs/value-simplify.ll:117
+  ; CHECK: tail call void @use(i32 %select-not-same-undef)
+  tail call void @use(i32 %select-not-same-undef)
+
----------------
uenoku wrote:
> jdoerfert wrote:
> > uenoku wrote:
> > > jdoerfert wrote:
> > > > uenoku wrote:
> > > > > jdoerfert wrote:
> > > > > > uenoku wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > This should arguably be
> > > > > > > >   `; CHECK: tail call void @use(i32 %phi-not-same)`
> > > > > > > > The pessimistic fixpoint should probably not revert to `nullptr` and cause a cascading failure but instead just go back to the original value.
> > > > > > > >
> > > > > > > > This should arguably be
> > > > > > > > 
> > > > > > > > `; CHECK: tail call void @use(i32 %phi-not-same)`
> > > > > > > In genericTraversal, `%phi-not-same` is  decomposed to i32 0, 1 so I think it is not currect.
> > > > > > > 
> > > > > > > > The pessimistic fixpoint should probably not revert to nullptr and cause a cascading failure but instead just go back to the original value.
> > > > > > > It looks good.
> > > > > > so `%phi-not-same` is not simplified, right?
> > > > > > But `%select-not-same-undef` is something or `undef` and then we should simplify it to "something". That would probably happen just fine if we do not use `nullptr` as pessimistic value but the original value itself. That is, the worst thing you can simplify an instruction to is itself.
> > > > > > so %phi-not-same is not simplified, right?
> > > > > Yes
> > > > > > But %select-not-same-undef is something or undef and then we should simplify it to  "something". 
> > > > > In the simplification of %select-not-same-undef, traversed values are `0, 1, undef` and we don't look at %phi-not-same variable itself.
> > > > > 
> > > > > Anyway, I changed to return original value in the pessimistic state and simplified value(always not equals to original value ) in the optimistic state.
> > > > > 
> > > > > In the simplification of %select-not-same-undef, traversed values are 0, 1, undef and we don't look at %phi-not-same variable itself.
> > > > 
> > > > But the possible values for  %select-not-same-undef should be [%phi-not-same, undef]. which we can simplify to %phi-not-same. 
> > > If so we need to change `GenericValueTraversal` a bit.
> > Why? What if the `VisitValueCB` would never return false?
> I think `GenericValueTraversal` is working in following way:
> ```
> worklist : [%select-not-same-undef]
> worklist : [%phi-not-same, undef]
> worklist : [i32 1, i32 0, undef]
> VisitValueCB(1) ; Simplified Value None -> i32 1
> VisitValueCB(0) ; Simplified Value i32 1 -> %select-not-same-undef (revert to orignal)
> ```
> Then we never call `VisitValueCB(%phi-not-same)`.
> If we want to simplify the value to `%phi-not-same`, we have to change to traverse not leaves but nodes (only direct children).
> 
> ```
> worklist : [%phi-not-same, undef]
> VisitValueCB(%phi-not-same) ; Simplified Value None -> %phi-not-same
> VisitValueCB(undef) ; Simplified Value doesn't change
> ```
> 
> 
I get it now.

We could *not* use the `GenericValueTraversal` as we actually want to look at selects and phis here. Or we need to track more state and extend `GenericValueTraversal`. Let's postpone this for a follow up.


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

https://reviews.llvm.org/D66967