[PATCH] D86798: [EarlyCSE] fold commutable intrinsics

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.
Herald added a subscriber: danielkiss.

In D86798#2244981 <https://reviews.llvm.org/D86798#2244981>, @nikic wrote:

>> 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?


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

https://reviews.llvm.org/D86798