[PATCH] D33037: [IfConversion] Keep the CFG updated incrementally in IfConvertTriangle

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added a comment.

In https://reviews.llvm.org/D33037#751100, @iteratee wrote:

> In https://reviews.llvm.org/D33037#750855, @uabelho wrote:
>
> > I have on purpose not attacked IfConvertSimple, IfConvertForkedDiamond
> >  and IfConvertDiamond to keep the size of the patch down, and I haven't seen any
> >  test case where they actually go wrong, but I think they suffer from similar
> >  problems since they also use RemoveExtraEdges/analyzeBranch to fix the CFG at
> >  the end.
>
>
> IfConvertForkedDiamond only applies to analyzable branches, so it should be safe.


Possible. I haven't tried to provoke it to miscompile due to unanalyzable instructions.

> If you change the others, it would be good to change it as well for consistency.

Yes I think so too.

In general I think we should try to avoid uses of analyzeBranch to fix things since it cannot be
trusted in all cases anyway. Doing optimizations based on its result is no problem but the use
in RemoveExtraEdges is problematic.

> I'll leave time for others to comment, but overall it looks fine to me.

Thanks!


https://reviews.llvm.org/D33037