[PATCH] D38566: [SimplifyCFG] don't sink common insts too soon (PR34603)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D38566#911458, @hfinkel wrote:

> In https://reviews.llvm.org/D38566#910739, @spatel wrote:
>
> > Patch updated:
> >  Now that we have SimplifyCFGOptions exposed to the pass managers, we can use a precise disablement of sinking common instructions to only the first instantiation of -simplifycfg (the one that runs before early-cse).
>
>
> Why? The selects also hurt GVN, etc. I thought we were going to delay this until the end of the canonicalization phase. Only then might be have more "phases" in this sense.


I might be overcautious here, but as I wrote in the last comment, I'd like to at least try to manufacture a GVN test case to show that difference. Also, I was hoping to get this diff in independently to reduce the risk of revert from unintended consequences from changing behavior in the later simplifycfg instantiations. If you feel more confident, I can flip the parameter in more places (or invert the default setting) in this patch.

> 
> 
>> Depending on your perspective, this is either a redefinition of "canonical form" or we've created a new "pre-canonical form".
>> 
>> Given the comments in PR34603, I suspect we want to limit simplifycfg sinking even more, but that should be a follow-up patch with more phase ordering tests, so we know exactly what kind of patterns we're affecting.




https://reviews.llvm.org/D38566