Page MenuHomePhabricator

InstCombine: Preserve nuw when reassociating nuw ops [1/3]
ClosedPublic

Authored by arsenm on Oct 30 2017, 6:40 AM.

Details

Summary

Alive says this is OK

Diff Detail

Event Timeline

arsenm created this revision.Oct 30 2017, 6:40 AM
efriedma added a subscriber: efriedma.
nlopes edited edge metadata.Nov 22 2017, 10:51 AM

This patch LGTM, except for the changes in tryFactorization(). It seems there's some code missing.

https://rise4fun.com/Alive/L2c

lib/Transforms/InstCombine/InstructionCombining.cpp
591

This variable is never used. It's defined over the next few lines, but never used to set NUW on the instruction.

arsenm updated this revision to Diff 202704.Jun 3 2019, 6:41 AM

Fix tryFactorization and fix missing test. Surprisingly there seem to be no restriction on the constant value for nuw, like nsw has?

Note for the tryFactorization part, alive times out without a specific type: https://rise4fun.com/Alive/7QM
It works if I specify i8: https://rise4fun.com/Alive/Thy

lebedev.ri added inline comments.Jun 18 2019, 11:54 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
198–231

Why does MaintainNoSignedWrap() need all that logic to prove that no overflow happens,
but MaintainNoUnsignedWrap() does nothing of sorts?

arsenm marked an inline comment as done.Jun 18 2019, 12:05 PM
arsenm added inline comments.
lib/Transforms/InstCombine/InstructionCombining.cpp
198–231

I'm not really sure why MaintainNoSignedWrap has all of those checks. It seems to me like it's looking an overflow that was already illegal?

This consists of 3 separate changes, it would be easier to review them separately.

lib/Transforms/InstCombine/InstructionCombining.cpp
198–231
226

Maintain is the wrong term then, has NUW flag or something

test/Transforms/InstCombine/reassociate-nuw.ll
1

Can you please precommit using utils/update_test_checks.py ?

arsenm updated this revision to Diff 205463.Jun 18 2019, 4:24 PM
arsenm marked an inline comment as done.
arsenm retitled this revision from InstCombine: Preserve nuw when reassociating nuw ops to InstCombine: Preserve nuw when reassociating nuw ops [1/3].

I think this has the wrong diff, this one is identical to D63528.

lebedev.ri requested changes to this revision.Jun 23 2019, 2:59 PM

(just marking as reviewed)

This revision now requires changes to proceed.Jun 23 2019, 2:59 PM
arsenm updated this revision to Diff 206215.Jun 24 2019, 7:41 AM

Correct diff

lebedev.ri accepted this revision.Jun 24 2019, 9:27 AM

Okay, this looks good to me.
Thanks!

lib/Transforms/InstCombine/InstructionCombining.cpp
338–344

I would still recomment

bool IsNUW = hasNoUnsignedWrap(I) && hasNoUnsignedWrap(*Op0);
bool IsNSW = MaintainNoSignedWrap(I, B, C);

ClearSubclassDataAfterReassociation(I);

if (IsNUW)
  I.setHasNoUnsignedWrap(true);
This revision is now accepted and ready to land.Jun 24 2019, 9:27 AM
arsenm closed this revision.Jun 24 2019, 2:37 PM
arsenm marked an inline comment as done.

r364233