This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Propagate the nuw for combine of add+mul
ClosedPublic

Authored by Allen on Aug 26 2022, 6:55 PM.

Details

Summary

As the commit of D132658, make the 'nuw' change separately.

Diff Detail

Event Timeline

Allen created this revision.Aug 26 2022, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2022, 6:55 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
Allen requested review of this revision.Aug 26 2022, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2022, 6:55 PM

Please include a general proof link to Alive2.

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

To be safe, use "dyn_cast" here. I don't have an example, but it is possible that the builder can constant fold the multiply, so the NewMul is not a BinaryOperator at this point.

llvm/test/Transforms/InstCombine/mul.ll
628

Do we have tests where only the add or only the mul has nuw?

Allen updated this revision to Diff 456181.Aug 28 2022, 3:37 AM
  1. add new test case with only mull or add has nuw
  2. add checking dyn_cast<BinaryOperator>(NewMul)
spatel accepted this revision.Aug 28 2022, 6:06 AM

LGTM - see inline comment for minor change.

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

It is better to use the normal LLVM code pattern instead of using both dyn_cast and cast:

if (auto *NewMulBO = dyn_cast<BinaryOperator>(NewMul))
  NewMulBO->setHasNoUnsignedWrap();
This revision is now accepted and ready to land.Aug 28 2022, 6:06 AM
Allen updated this revision to Diff 456183.Aug 28 2022, 7:23 AM

Address comment to use the normal LLVM code pattern

This revision was landed with ongoing or failed builds.Aug 28 2022, 8:01 AM
This revision was automatically updated to reflect the committed changes.