This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fix missing nsw flag when fold -(x-y) to y-x
AbandonedPublic

Authored by bcl5980 on Mar 23 2022, 2:05 AM.

Details

Summary

Use CreateNSWSub instead of CreateSub if possible

Diff Detail

Event Timeline

bcl5980 created this revision.Mar 23 2022, 2:05 AM
bcl5980 requested review of this revision.Mar 23 2022, 2:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 2:05 AM
nikic requested changes to this revision.Mar 23 2022, 2:18 AM

This is correct if both subs are nsw: https://alive2.llvm.org/ce/z/r6UV8Q But not if only one of them is: https://alive2.llvm.org/ce/z/3_2BDj

This revision now requires changes to proceed.Mar 23 2022, 2:18 AM
lebedev.ri requested changes to this revision.Mar 23 2022, 3:42 AM

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

This revision now requires changes to proceed.Mar 23 2022, 3:42 AM
xbolva00 added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
248

CreateSub(I->getOperand(1), I->getOperand(0), I->getName() + ".neg", false, I->hasNoSignedWrap() && HasNSW); ?

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

It comes from here https://reviews.llvm.org/D122013
Maybe we can enable it when IsTrulyNegation ? Abandon is also OK for me.

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

It comes from here https://reviews.llvm.org/D122013

Maybe we can enable it when IsTrulyNegation ?

Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
But even then, the really big problem for NSW preservation is that every instruction inbetween that we manage to negate through affects the NSW: https://alive2.llvm.org/ce/z/4A--JC

Abandon is also OK for me.

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

It comes from here https://reviews.llvm.org/D122013

Maybe we can enable it when IsTrulyNegation ?

Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
But even then, the really big problem for NSW preservation is that every instruction inbetween that we manage to negate through affects the NSW: https://alive2.llvm.org/ce/z/4A--JC

Abandon is also OK for me.

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

It comes from here https://reviews.llvm.org/D122013

Maybe we can enable it when IsTrulyNegation ?

Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
But even then, the really big problem for NSW preservation is that every instruction inbetween that we manage to negate through affects the NSW: https://alive2.llvm.org/ce/z/4A--JC

Abandon is also OK for me.

Thanks for the detail explaination. So if we really want this work, it should be

bool NSW = I->hasNoSignedWrap() && HasNSW && IsTrulyNegation && Depth == 0;
return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
                         I->getName() + ".neg", false, NSW);

or we need to trace every recursive instruction is also NSW, am I right?

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

It comes from here https://reviews.llvm.org/D122013

Maybe we can enable it when IsTrulyNegation ?

Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
But even then, the really big problem for NSW preservation is that every instruction inbetween that we manage to negate through affects the NSW: https://alive2.llvm.org/ce/z/4A--JC

Abandon is also OK for me.

Must we do this? I was really hoping we wouldn't.
Does this actually unblock some measurable improvement?

Consider: https://alive2.llvm.org/ce/z/XYfMSm
Both subs have NSW, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6

It comes from here https://reviews.llvm.org/D122013

Maybe we can enable it when IsTrulyNegation ?

Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
But even then, the really big problem for NSW preservation is that every instruction inbetween that we manage to negate through affects the NSW: https://alive2.llvm.org/ce/z/4A--JC

Abandon is also OK for me.

Thanks for the detail explaination. So if we really want this work, it should be

bool NSW = I->hasNoSignedWrap() && HasNSW && IsTrulyNegation && Depth == 0;
return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
                         I->getName() + ".neg", false, NSW);

or we need to trace every recursive instruction is also NSW, am I right?

I believe that is the idea, yes.

Thanks again @lebedev.ri. I will update the code to

bool NSW = I->hasNoSignedWrap() && HasNSW && IsTrulyNegation && Depth == 0;
return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
                         I->getName() + ".neg", false, NSW);

but if there is no other comments I prefer to abandon it.

bcl5980 updated this revision to Diff 417575.Mar 23 2022, 5:39 AM
bcl5980 edited the summary of this revision. (Show Details)

only depth == 0 enable nsw flag

I'm fine with abandoning the code change. I just wanted to make sure we were not miscompiling in D122013.

I would commit the tests anyway with some comments or a link back to this review (in case this comes up again in the future in some other context).

bcl5980 abandoned this revision.Mar 23 2022, 6:48 AM