Page MenuHomePhabricator

GlobalISel: Assume G_INTRINSIC* are convergent
ClosedPublic

Authored by arsenm on Jan 20 2020, 4:32 AM.

Details

Summary

This is safer in case anyone tries to run MI optimization passes on
pre-selected MIR. If there turns out to be a real reason to do this,
we might need to add separate convergent intrinsic opcodes.

Diff Detail

Event Timeline

arsenm created this revision.Jan 20 2020, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 4:32 AM

Seems like a good idea to be correct (assume convergent unless isn't).

This revision is now accepted and ready to land.Feb 4 2020, 12:26 PM

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.

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.

My understanding with this patch was let's go with this (and be correct ie don't sink instructions so you don't break semantics) and if it ends up being a problem, we'll add another intrinsic opcode.

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.

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.

arsenm added a comment.Feb 4 2020, 1:32 PM

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.

Eventually, but none of these are problems I want to work on solving right now. My goal is to avoid subtle problems in the future, so I still think we should just go with this for now.

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.

Eventually, but none of these are problems I want to work on solving right now. My goal is to avoid subtle problems in the future, so I still think we should just go with this for now.

Ok, we'll revisit this later then.