[PATCH] D93065: [InstCombine] Disable optimizations of select instructions that causes propagation of poison values

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

> I think we'll just have to bite the bullet and do that. In a systematic way, before this lands. I think that the number of places that need to be changed is actually rather limited as we're only interested in and/or on booleans. We don't have that many analyses dealing with conditions.

+1

> Worth noting is that some of the code deleted in this patch should be retained as a canonicalization. E.g. a ? a : b should become a ? true : b, a ? b : true should become !a ? true : b, etc. That way the remaining code only has to deal with canonical select patterns.

Yes, this makes sense to me.

> It may also be worthwhile to convert the special cases of icmp (a, X) ? (icmp a, Y) : false to icmp (a, X) & (icmp a, Y), though I'm not sure if that is even useful if we handle the select pattern reasonable everywhere else.

I think this is helpful because it helps the expression syntactically show more information. Poison propagation is more aggressive in the latter case.
If we want to support this transformation, I think having something like `impliesPoison` in ValueTracking and calling it when doing this conversion will be helpful. I locally have an old patch that does this (maybe somewhere in Phabricator too).
What do you think about this workflow:
(1) Add the LogicalOr/And pattern match, make a few important analysis (LVI, transformations in ValueTracking) as well transformations like GVN support select pattern.
(2) If necessary, add an analysis function that can be used for conditionally allowing select->and/or.
(3) See how the performance goes..!


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

https://reviews.llvm.org/D93065