[PATCH] D69498: IR: Invert convergent attribute handling

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds added a comment.

In D69498#2704364 <https://reviews.llvm.org/D69498#2704364>, @foad wrote:

>> This recasts the whole question to be one about combining optimizations with target-specific information. The only changes required are in transforms that check `CallInst::isConvergent()`. These should now also check `TTI`, possibly adding a dependency on the `TTI` analysis where it didn't exist earlier.
>
> @sameerds I agree with your conclusions but I would describe the situation a little differently. As I understand it, the optimizations that check isConvergent really only care about moving convergent calls past control flow //that might be divergent//. !hasBranchDivergence is a promise that there are no possible sources of divergence for a target, so you can run a divergence analysis if you like but it will just tell you that everything is uniform, so all control flow is uniform, so it's OK to move isConvergent calls around.
>
> In practice the optimizations that check isConvergent don't seem to use divergence analysis, they just pessimistically assume that any control flow might be divergent (if hasBranchDivergence). But they could and perhaps should use divergence analysis, and then it would all just fall out in the wash with no need for an explicit hasBranchDivergence test.

Sure it is formally the same thing. But in practice, the main issue for everyone is the effect on compile time for targets that don't care about convergence/divergence. For such targets, running even the divergence analysis is an unnecessary cost. Any checks for uniform control flow will still be hidden under TTI.hasBranchDivergence() for such targets. We see that happening already in places where divergence analysis is constructed optionally.


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

https://reviews.llvm.org/D69498