[PATCH] D45378: [InstCombine] Propagate null values from conditions to other basic blocks

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D45378#1060265, @xbolva00 wrote:

> In https://reviews.llvm.org/D45378#1060146, @lebedev.ri wrote:
>
> > Note that such a transform (`null` propagation) is already being done somewhere. https://godbolt.org/g/ow79c8
> >  I'd recommend to first look where that is happening, and extend that, not introduce a duplicate-but-better fold elsewhere.
>
>
> But it does not work in basic case.. https://godbolt.org/g/wtimXj


In *another* basic case. It is clearly working in the case i linked, no?

In https://reviews.llvm.org/D45378#1060270, @xbolva00 wrote:

> In https://reviews.llvm.org/D45378#1060239, @spatel wrote:
>
> > This is pushing instcombine beyond local simplifications. Propagating values across blocks should probably be handled in CVP (-correlated-propagation).
>


Take working example, save (slight manual cleaning required) it as `test.ll` F5947558: test.ll <https://reviews.llvm.org/F5947558>,
and run `$ opt -O2 -S test.ll -print-after-all` (https://reviews.llvm.org/D44244 isn't there still),
and look for when line `tail call void @bar(i8* %0)` is replaced with `tail call void @bar(i8* null)`
That will tell you which pass does it currently. (spoiler: `Global Value Numbering`, hmm...)

> Maybe, but here it also fits, every info required for this transformation is available here.
> 
> Implementation of this optimization would probably be quite massive in the CorrelatedValuePropagation.

But then you will have two similar folds doing essentially the same thing, but in different passes, and one is more broken than the other one.
So while this will may be easy to do right now, it is the wrong solution long-term, it will increase technical debt,
and someone later on will stumble into that and will have to either fix one of them, or deduplicate them...


https://reviews.llvm.org/D45378