[PATCH] D69498: IR: Invert convergent attribute handling

[Jay Foad via Phabricator via cfe-commits]

foad added a comment.

In D69498#2703317 <https://reviews.llvm.org/D69498#2703317>, @sameerds wrote:

> The way I see it, the notion of convergence is relevant only to a certain class of targets (usually represented by GPUs) and it only affects certain optimizations. Then why not have only these optimizations check `TTI` to see if convergence matters? `TTI.hasBranchDivergence()` seems like a sufficient proxy for this information.
>
> 1. `convergent` becomes the default in LLVM IR, but it does not affect optimizations on non-GPU targets.
> 2. This is not a reinterpretation of the same IR on different targets. The notional execution model of LLVM IR will say that all function calls are convergent. Targets that only care about one thread at a time represent the degenerate case where all executions are convergent anyway.
>
> 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.


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

https://reviews.llvm.org/D69498