[PATCH] D73030: GlobalISel: Assume G_INTRINSIC* are convergent

Tue Feb 4 13:18:59 PST 2020

aemerson added a comment.

In D73030#1857726 <https://reviews.llvm.org/D73030#1857726>, @arsenm wrote:

> In D73030#1857708 <https://reviews.llvm.org/D73030#1857708>, @aemerson wrote:
>
> > I'm not 100% sure on what the convergent attribute means here, but assuming that it means it's not allowed to sink instructions into control flow etc, I don't think we should be marking these generic intrinsic ops with it. On arm64 we can use G_INTRINSIC to represent user-written intrinsic instructions which have no problem with being made control dependent.
>
>
> The intrinsic opcodes need to conservatively assume it could call an intrinsic with any property. These can be used to call an intrinsic or call site marked as convergent, so they have to be convergent to be conservatively correct. Since nothing in the globalisel pipeline currently does any control flow sinking or anything, it's not worth the hassle of optimizing this by adding an additional 2 G_INTRINSIC* variants. Until that comes up, I'd rather leave this in the safe state so somebody doesn't silently break convergent handling at some point in the future.

The localizer is one pass that can sink instructions, although it currently doesn't do intrinsics yet.

If this change is made then CPU targets will need to create the new G_INTRINSIC_(DIVERGENT?) opcodes anyway, since not having sinkable intrinsics (like many vector ops, and some exotic scalar ones) won't be acceptable for optimization purposes. So I think we will need the two extra opcodes anyway. Unless we have a different mechanism to communicate this property, like an MI flag? Actually, the issue we had with "tail" attributes on the memcpy intrinsics might also benefit from finding a better solution to this properties issues.

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

https://reviews.llvm.org/D73030