This is an archive of the discontinued LLVM Phabricator instance.

[IR] Add WithOverflowInst class
ClosedPublic

Authored by nikic on Apr 14 2019, 7:28 AM.

Details

Summary

Related to D60656. This adds a WithOverflowInst class with a few helper methods to get the underlying binop, signedness and nowrap type and makes use of it where sensible. I think the result is pretty nice and there will be two more uses in D60650/D60656.

The refactorings are all NFC, though I left a couple of TODOs where things could be improved. In particular we have two places where add/sub are handled but mul isn't.

As a followup for this, I believe it would be good to add PatternMatch support for m_ExtractValue() and m_WithOverflow(), so you can write something like m_ExtractValue(m_WithOverflow(WO), 0), which is a common pattern.

Diff Detail

Event Timeline

nikic created this revision.Apr 14 2019, 7:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2019, 7:28 AM

Looks reasonable (hard to tell with large diffs).
I'm not sure if that's all it takes, so would be good for someone else to look through.

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
403

I'm not sure what is the prevalent pattern in LLVM, but t'm reasonably sure it's TODO:, FIXME: in general.

420

There were some setters in Instruction(?) that allowed to operate on NSW / NUW flags.

nikic updated this revision to Diff 195407.Apr 16 2019, 9:54 AM
nikic marked 2 inline comments as done.

Fix TODO style; use setHasNo(Un)SignedWrap().

lebedev.ri accepted this revision.Apr 16 2019, 10:15 AM

Ok, took me a couple passes, but i think *these* changes are good.
Don't know if there is more code to cleanup though.
Thanks.

llvm/lib/Analysis/ScalarEvolution.cpp
4584

If you don't intend to look into that, could you please file a bug, if there isn't one already?

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
403

Similarly, if there is no bug yet, could you please create one?

This revision is now accepted and ready to land.Apr 16 2019, 10:15 AM
This revision was automatically updated to reflect the committed changes.
nikic marked 3 inline comments as done.Apr 16 2019, 1:25 PM
nikic added inline comments.
llvm/lib/Analysis/ScalarEvolution.cpp
4584

Just looked into this one... The NUW/NSW flags set here are actually only used when constructing AddRecs, which of course doesn't apply to multiplies. So I think I'll just replace this TODO with a comment saying so.

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
403

For reference, this is D60791.