This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Support SMin/Umin for GetMinTrailingZeros
ClosedPublic

Authored by caojoshua on Jan 11 2023, 11:25 PM.

Details

Summary

There is already support for Max counterparts. No reason why Min should
not be supported.

NFC: code in GetMinTrailingZeroes is copied for a couple node types.
Refactor them into a single code block.

Diff Detail

Event Timeline

caojoshua created this revision.Jan 11 2023, 11:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 11:25 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
caojoshua requested review of this revision.Jan 11 2023, 11:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 11:25 PM
This comment was removed by caojoshua.

edit comment in test

Add a test where trip multiple is 1

nikic added inline comments.Jan 12 2023, 12:31 AM
llvm/lib/Analysis/ScalarEvolution.cpp
6342

While here, would you mind supporting UMinSeq as well?

6346–6347

Any reason to deviate from the previous structure?

caojoshua added inline comments.Jan 12 2023, 8:32 AM
llvm/lib/Analysis/ScalarEvolution.cpp
6342

Yeah sure. Let me test this later. I think we can simplify this to

isa<SCEVMinMaxExpr> || isa<SCEVSequentialMinMaxExpr>
6346–6347

i -> I because LLVM coding standards state that Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats). And cause my editor is smart enough to know we are using LLVM coding standards and recommend this change.

No need to assign assign getNumOperands() to E. Can just compare directly against getNumOperands(), and remove unnecessary E variable. M is const, so we know the number of operands will not change through iterations barring some unsafe casting (I think this is how C++ const works).

No strong opinions from me. This deviates from code style in rest of function. I can update the rest of the function as a NFC followup to make sure everything is consistent.

caojoshua updated this revision to Diff 488855.Jan 12 2023, 8:50 PM
  • support UMinSequential. Add a test (see note below)
  • single isa<SCEVMinMaxExpr>(S) instead of checking each subtype
  • fix code style based on comments

I am unable to create a testcase for GetMinTrailingZeroes on UMinSequential
SCEVs. I tried adapting some tests from https://reviews.llvm.org/D116766, but
however I modify tests, either the Trip Multiple is not returning what I would
expect, or the UMinSequential turns into a regular umin.

All existing integration tests pass. Not sure if we need more testing, or we
can support UMinSequential in a separate work.

caojoshua added inline comments.Jan 12 2023, 8:51 PM
llvm/lib/Analysis/ScalarEvolution.cpp
6346–6347

I reverted the previous structure, but kept the variable names capitalized

nikic accepted this revision.Jan 13 2023, 12:58 AM

LGTM

This revision is now accepted and ready to land.Jan 13 2023, 12:58 AM
This revision was landed with ongoing or failed builds.Jan 13 2023, 2:32 AM
This revision was automatically updated to reflect the committed changes.