Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

analysis for min/max/abs intrinsics #46259

Closed
rotateright opened this issue Jul 30, 2020 · 18 comments
Closed

analysis for min/max/abs intrinsics #46259

rotateright opened this issue Jul 30, 2020 · 18 comments
Labels
bugzilla Issues migrated from bugzilla llvm:optimizations

Comments

@rotateright
Copy link
Contributor

Bugzilla Link 46915
Version trunk
OS All
Depends On #48244
Blocks #45317
CC @topperc,@davezarzycki,@davidbolvansky,@DMG862,@LebedevRI,@RKSimon,@nikic

Extended Description

We recently added IR intrinsics for min/max/abs, and there are several areas that need to be updated to support and optimize these. Please comment here if you are or are planning to work on any part of this, so we do not duplicate effort.

Partial list:

  • ConstantFolding
  • ValueTracking
  • InstSimplify
  • InstCombine
  • PatternMatch
  • DemandedBits
  • Vectorization
  • Cost Modeling
  • LVI, CVP, SCCP
  • SCEV
  • IndVars
@rotateright
Copy link
Contributor Author

I'm working on ConstantFolding and InstSimplify currently.

@topperc
Copy link
Collaborator

topperc commented Jul 30, 2020

I'm working on computeKnownBits for abs

@RKSimon
Copy link
Collaborator

RKSimon commented Jul 30, 2020

Default + X86 costs have been committed.

@RKSimon
Copy link
Collaborator

RKSimon commented Aug 7, 2020

SLP - basic abs/min/max vectorization works fine

I'm going to look at adding min/max intrinsic reduction support

@rotateright
Copy link
Contributor Author

This was the last InstSimplify that was on my list to adapt:
https://reviews.llvm.org/rG1470ce4a76fc

Constant folding should be complete too.

@rotateright
Copy link
Contributor Author

I think we have most of it covered now. I'm not as familiar with the last 3 on the list (LVI/CVP/SCCP, SCEV, IndVars).

What needs to be done before we start producing intrinsics in place of cmp+select in instcombine transforms?

@nikic
Copy link
Contributor

nikic commented Sep 3, 2020

The main thing that still needs to be done is recognizing min/max intrinsics in SCEV.

@rotateright
Copy link
Contributor Author

First try to get abs() canonicalized:
https://reviews.llvm.org/D87188

As I mentioned there, I don't think we have our size cost models updated (but let me know if I'm misreading the code).

We updated the throughput models here:
c5ef1f1
0a0f282

That's what is used by the vectorizers, but that's probably not what is used by unrolling, inlining, and simplifycfg.

@rotateright
Copy link
Contributor Author

The basic cost model handling is in place as of:
https://reviews.llvm.org/D90554 / https://reviews.llvm.org/rGe32bd3512043

Still trying to get abs canonicalization in with:
https://reviews.llvm.org/D87188

@LebedevRI
Copy link
Member

The basic cost model handling is in place as of:
https://reviews.llvm.org/D90554 / https://reviews.llvm.org/rGe32bd3512043

Still trying to get abs canonicalization in with:
https://reviews.llvm.org/D87188
Done.

@nikic
Copy link
Contributor

nikic commented Dec 18, 2020

Awesome! Only part left now is to canonicalize min/max...

I took a quick look at that, here's how the InstCombine test diff looks like: https://gist.github.com/nikic/f69061736ea8e6b6c159b28b09991599

We're still missing quite a few folds, e.g. pushing nots across min/max and forming unsigned saturating math.

@rotateright
Copy link
Contributor Author

Over in bug 48816, we discovered that we need narrowing combines for abs to avoid regressions.

I'll work on the related folds for min/max.

@rotateright
Copy link
Contributor Author

I'll work on the related folds for min/max.

Those were:
https://reviews.llvm.org/rG09a136bcc694
https://reviews.llvm.org/rG0ce2920f1707

@rotateright
Copy link
Contributor Author

This is likely the best view of current state:
https://reviews.llvm.org/D98152

There are still missing parts of analysis/instcombines as noted.

I think SLP is done (and the sooner we can switch to intrinsics, the better because supporting cmp+select is still at risk for hard-to-spot bugs as shown in https://reviews.llvm.org/D99753 ).

AFAIK, LoopVectorizer is partially done. I'll add some PhaseOrdering tests to increase visibility.

@LebedevRI
Copy link
Member

Added CVP handling for abs/min/max

@davezarzycki
Copy link
Member

What about saturating intrinsics? The same pattern matching discrepancy seems to apply, no?

@fhahn
Copy link
Contributor

fhahn commented Nov 27, 2021

mentioned in issue #48244

@nikic
Copy link
Contributor

nikic commented Mar 1, 2022

Closing this as completed. Remaining issues should be reported separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla llvm:optimizations
Projects
None yet
Development

No branches or pull requests

8 participants