[PATCH] D45862: [InstCombine] Relax restriction in foldSelectInstWithICmp for sake of smaller code size

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D45862#1074397, @lebedev.ri wrote:

> In https://reviews.llvm.org/D45862#1073841, @spatel wrote:
>
> > I don't think we should be doing any of these select-of-constant transforms in instcombine.
> >
> > It's worse for code analysis, more IR instructions, and may be detrimental to perf. Think about the cases where a conditional move executes at the same speed as a simple add (Ryzen?) or we have profile data for the compare, so branch prediction is perfect.
> >
> > There's lots of code that does this kind of thing in the DAG, and that's where I think it belongs (using target hooks as needed). There was some discussion about this on llvm-dev here:
> >  https://groups.google.com/forum/#!topic/llvm-dev/pid_thv2X-A
> >
> > So I think we should be removing some of these transforms from instcombine rather than adding to them.
>
>
> There is a second side to this though.
>
> Even if all such "performance-degrading" transforms are removed from instcombine
>  (yes, i think the pass is rather huge and monolithic, this is a problem), it won't solve the problem.
>  The same 'bad' patterns surely could be produced via some other way.
>
> I don't see how this is any different than lowering intrinsics.
>  Of course, they are no longer intrinsics, but IR, so optimizations may
>  break their canonical form, and backend will need to be adjusted
>  to recognize the IR patters (and potentially recognize the patterns
>  that did not originate form intrinsics, which is great).
>
> So i'd say this really is backend's (DAGCombine?) problem.
>  This is just a question of whether or not there is interest to have a high quality codegen,
>  regardless of the input. It should recognize the 'bad' patterns, and if profitable,
>  transform to improve 'performance'. In this case, back to select.
>
> And yes, absolutely, this is more complicated than just "stop dealing with it in instcombine" :)


I agree. Let me clarify the suggestion: don't just remove these control-flow to data-flow transforms from instcombine. Canonicalize to the cmp+select form. And then let the DAG convert it back if it's profitable. I did some fraction (what I thought were the most common parts) of this job already following the cited llvm-dev discussion. Here's the direct link to that reply:
http://lists.llvm.org/pipermail/llvm-dev/2016-September/105373.html


https://reviews.llvm.org/D45862