This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Use context to strengthen flags of BinOps
ClosedPublic

Authored by mkazantsev on Jul 13 2022, 7:31 AM.

Details

Summary

Sometimes SCEV cannot infer nuw/nsw from something as simple as

  len in [0, MAX_INT]
...
  iv = phi(0, iv.next)
  guard(iv <s len)
  guard(iv <u len)
  iv.next = iv + 1

just because flag strenthening only relies on definition and does not use local facts.
This patch adds support for the simplest case: inference of flags of add(x, constant)
if we can contextually prove that x <= max_int - constant.

In case if it has negative CT impact, there is an option to disable this logic. I woudln't
expect that though.

Diff Detail

Event Timeline

mkazantsev created this revision.Jul 13 2022, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 7:31 AM
mkazantsev requested review of this revision.Jul 13 2022, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 7:31 AM

If it's too bad we can have the option switched off by default. I was going to spend some time improving our context-based analyzes (looks like we do redundant traversal), but haven't done it yet.

The change seems reasonable.

One potentially concerning thing is if we start using context for inferring no-wrap flags, we theoretically can complete the "there's a check => doesn't overflow, doesn't overflow => doesn't need the check" circle. But even if we do, it seems, the problem would be on the side of the transformation, removing checks based on flags.

Please wait few more days and then feel free to land if no one objects.

There is a subtlety between no-wrap flags in instructions and SCEVs. The short answer is - no, correct usage of SCEV should never lead to what you are describing. We had such bugs in the past because SCEV was misused.

apilipenko accepted this revision.Aug 2 2022, 1:55 PM
apilipenko added a subscriber: apilipenko.
apilipenko added inline comments.
llvm/lib/Analysis/ScalarEvolution.cpp
2330

Maybe add a comment explaining the pattern you are looking for here?

Something like: "We can prove that add(x, constant) doesn't wrap if isKnownPredicateAt can guarantee that x <= max_int - constant at the given context."

This revision is now accepted and ready to land.Aug 2 2022, 1:55 PM
mkazantsev marked an inline comment as done.Aug 2 2022, 11:25 PM
This revision was landed with ongoing or failed builds.Aug 3 2022, 12:09 AM
This revision was automatically updated to reflect the committed changes.

It did show some CT drops, but I think the change is useful enough to have it. If not, let's consider switching the option off, but I would advice against that.