Page MenuHomePhabricator

[EarlyCSE] fold commutable intrinsics
ClosedPublic

Authored by spatel on Aug 28 2020, 11:28 AM.

Details

Summary

Handling the new min/max intrinsics is the motivation, but it turns out that we have a bunch of other intrinsics with this missing bit of analysis too.

The FP min/max tests show that we are intersecting FMF, so that should be safe.

As noted in https://llvm.org/PR46897 , there is a commutative property specifier for intrinsics, but no corresponding function attribute, and so apparently no uses of that bit. I can remove that here or independently if this is an adequate or better solution.

Diff Detail

Event Timeline

spatel created this revision.Aug 28 2020, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 11:28 AM
spatel requested review of this revision.Aug 28 2020, 11:28 AM
lebedev.ri accepted this revision.Aug 28 2020, 11:37 AM

LG to me, thanks

This revision is now accepted and ready to land.Aug 28 2020, 11:37 AM

As noted in https://llvm.org/PR46897 , there is a commutative property specifier for intrinsics, but no corresponding function attribute, and so apparently no uses of that bit. I can remove that here or independently if this is an adequate or better solution.

I think the basic approach here is fine. Dropping the confusing intrinsic property once this lands sounds good.

The only suggestion I would make is to make the primary Instruction::isCommutative() method actually forward to IntrinsicInst::isCommutative() for intrinsics. Having I->isCommutative() and cast<IntrinsicInst>(I)->isCommutative() report different things is confusing. I think if you do this, then we will also get commutation support in GVN for free.

As noted in https://llvm.org/PR46897 , there is a commutative property specifier for intrinsics, but no corresponding function attribute, and so apparently no uses of that bit. I can remove that here or independently if this is an adequate or better solution.

I think the basic approach here is fine. Dropping the confusing intrinsic property once this lands sounds good.

The only suggestion I would make is to make the primary Instruction::isCommutative() method actually forward to IntrinsicInst::isCommutative() for intrinsics. Having I->isCommutative() and cast<IntrinsicInst>(I)->isCommutative() report different things is confusing. I think if you do this, then we will also get commutation support in GVN for free.

Ah, good point, but I'll need to make some updates in GVN at least because it has:

assert(I->getNumOperands() == 2 && "Unsupported commutative instruction!");

...and I'm seeing regression test failures.
Or I can just leave the 'fix' intrinsics off of the list here for now. Do you have a preference?

Or I can just leave the 'fix' intrinsics off of the list here for now. Do you have a preference?

Yeah, I think it would be best to drop the fixpoint intrinsics for now, as they aren't commutative in the usual sense and we won't be able to exploit that in the current implementation anyway.

Or I can just leave the 'fix' intrinsics off of the list here for now. Do you have a preference?

Yeah, I think it would be best to drop the fixpoint intrinsics for now, as they aren't commutative in the usual sense and we won't be able to exploit that in the current implementation anyway.

Just dropping the fixed-point intrinsics from this patch is not enough because this assert still fires for the 2 operand intrinsics:

assert(I->getNumOperands() == 2 && "Unsupported commutative instruction!");

...because a CallInst's first operand is the called function (the 2 argument intrinsics always have 3 operands).
So at the least, I need to adjust the asserts in GVN and NewGVN. There's also a use of isCommutative() in SLP that I can't tell at a glance is safe. Still want to make those changes here, or a follow-up patch?

nikic accepted this revision.Aug 28 2020, 1:58 PM

Just dropping the fixed-point intrinsics from this patch is not enough because this assert still fires for the 2 operand intrinsics:

assert(I->getNumOperands() == 2 && "Unsupported commutative instruction!");

...because a CallInst's first operand is the called function (the 2 argument intrinsics always have 3 operands).
So at the least, I need to adjust the asserts in GVN and NewGVN. There's also a use of isCommutative() in SLP that I can't tell at a glance is safe. Still want to make those changes here, or a follow-up patch?

Ah, that's unfortunate. Let's handle this in a followup then.

This revision was automatically updated to reflect the committed changes.