This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Propagate nsw flag when negating
ClosedPublic

Authored by nikic on Aug 22 2023, 6:43 AM.

Details

Summary

When pushing a sub nsw 0, %x negation into an expression, try to preserve the nsw flag for the cases where this is possible. Do this by passing the flag through recursive Negator::negate() calls.

Proofs: https://alive2.llvm.org/ce/z/oRPNcY

Diff Detail

Event Timeline

nikic created this revision.Aug 22 2023, 6:43 AM
nikic requested review of this revision.Aug 22 2023, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 6:43 AM

All of these except the -1 << X case work for NUW as well: https://alive2.llvm.org/ce/z/zzPZRQ
Maybe instead of a bool pass a bitmask of flags? Would make it easier to add NUW support in a followup.

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
264

You can have NSW on this mul given that Op1 is a constant.

llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
241

Nit: I would rename IsNSW to MayNSW or ParentNSW as that seems to more accurately represent the usage.

goldstein.w.n added inline comments.Aug 22 2023, 9:41 AM
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
306

Why does freeze kill nsw?

All of these except the -1 << X case work for NUW as well: https://alive2.llvm.org/ce/z/zzPZRQ
Maybe instead of a bool pass a bitmask of flags? Would make it easier to add NUW support in a followup.

sub nuw 0, %x is only non-poison when %x is zero. For that reason, it's not really something that occurs in practice, so I don't think it's worth handling.

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
264

I don't think you can add nsw on the mul if I got this right: https://alive2.llvm.org/ce/z/lSk99J

Though it looks like passing nsw to the negator would be okay: https://alive2.llvm.org/ce/z/MrwT5K

llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
306

Hm, looks like it's okay to keep nsw: https://alive2.llvm.org/ce/z/vnYqoi That's quite unexpected to me.

goldstein.w.n added inline comments.Aug 22 2023, 11:59 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
264

You have it right, the idea though is that since Op1 is a constant we can infer when it would be NSW i.e:
https://alive2.llvm.org/ce/z/6JPtJR

goldstein.w.n added inline comments.Aug 22 2023, 1:36 PM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
264

edit: Actually you can just propagate the NSW flag i.e:
https://alive2.llvm.org/ce/z/Xw992v

nikic updated this revision to Diff 552640.Aug 23 2023, 2:49 AM
nikic marked 4 inline comments as done.
nikic edited the summary of this revision. (Show Details)

Handle freeze, handle nsw in mul by neg pow 2 fold.

nikic added inline comments.Aug 28 2023, 5:59 AM
llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
241

The intention here was that IsNSW refers to whether the negation is nsw.

This revision is now accepted and ready to land.Sep 13 2023, 10:55 PM
This revision was landed with ongoing or failed builds.Sep 14 2023, 12:09 AM
This revision was automatically updated to reflect the committed changes.