Page MenuHomePhabricator

[InstCombine] Canonicalize SPF to min/max intrinsics
Needs ReviewPublic

Authored by nikic on Mar 7 2021, 1:21 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None

Diff Detail

Unit TestsFailed

TimeTest
180 msx64 windows > Clang.CodeGen::builtins-wasm.c
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16c2-1\llvm-project\premerge-checks\build\lib\clang\13.0.0\include -nostdsysteminc -triple wasm32-unknown-unknown -target-feature +unimplemented-simd128 -target-feature +nontrapping-fptoint -target-feature +exception-handling -target-feature +bulk-memory -target-feature +atomics -flax-vector-conversions=none -O3 -emit-llvm -o - C:\ws\w16c2-1\llvm-project\premerge-checks\clang\test\CodeGen\builtins-wasm.c | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\clang\test\CodeGen\builtins-wasm.c -check-prefixes WEBASSEMBLY,WEBASSEMBLY32

Event Timeline

nikic created this revision.Mar 7 2021, 1:21 PM
nikic requested review of this revision.Mar 7 2021, 1:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2021, 1:21 PM
nikic added inline comments.Mar 7 2021, 1:56 PM
llvm/test/Transforms/InstCombine/2007-12-18-AddSelCmpSub.ll
24

Not clear to me whether these are regressions or not. Depends on how much we prefer to have min/max over other select structures.

llvm/test/Transforms/InstCombine/adjust-for-minmax.ll
250

We explicitly fold min/max to work on the narrower type, so I'm counting these as improvements.

llvm/test/Transforms/InstCombine/div-shift.ll
66

Regression

llvm/test/Transforms/InstCombine/max-of-nots.ll
9

Regression

llvm/test/Transforms/InstCombine/max_known_bits.ll
40

Regression (missing nsw flag)

llvm/test/Transforms/InstCombine/min-positive.ll
11

Regression

llvm/test/Transforms/InstCombine/minmax-demandbits.ll
8

Regression

llvm/test/Transforms/InstCombine/minmax-fold.ll
283

Regression

739

Regression

887

This looks neutral.

1103

Regression

1351

Regression

llvm/test/Transforms/InstCombine/sadd_sat.ll
15

Regression

llvm/test/Transforms/InstCombine/sub-minmax.ll
23

Regression

llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-expanded.ll
153

No longer vectorized?

spatel added a subscriber: spatel.Mar 9 2021, 9:47 AM

Thanks for sharing this! I added a couple of 'not' op enhancements to instcombine, so we should be slightly closer now.

llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-expanded.ll
153

This requires rework in SLP. It assumes that min/max patterns are only in cmp+sel form. I made it work for FP (maxnum/minnum) already, so I'll take a look.

nikic added a comment.Mar 9 2021, 10:03 AM

Thanks for sharing this! I added a couple of 'not' op enhancements to instcombine, so we should be slightly closer now.

Thanks! I've updated with new results. It looks like we still need a few more folds for not to reach parity.

llvm/test/Transforms/InstCombine/max-of-nots.ll
9

Fixed

89

Still regresses

118

Still regresses

209

Still regresses -- in this case we'd want to push the not into NOT_VALUE and drop the other two.

370

These all still regress

RKSimon added inline comments.
llvm/test/Transforms/PhaseOrdering/minmax.ll
23

Any ideas whats causing this code bloat?

spatel added inline comments.Mar 15 2021, 7:50 AM
llvm/test/Transforms/PhaseOrdering/minmax.ll
23

We probably need to implement some reassociation folds for the intrinsics like instcombine's factorizeMinMaxTree():
https://github.com/llvm/llvm-project/blob/da55af7f1d348c133774d8e8117d60462363fef5/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L2222

D88287 might also help, but NaryReassociate isn't in the default optimization pipeline?

spatel added inline comments.Mar 23 2021, 8:38 AM
llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-expanded.ll
153

SLP and test updated:
3c8473ba534d / 9d45daf4656e
...so this diff should be gone now.

spatel added inline comments.Mar 28 2021, 10:45 AM
llvm/test/Transforms/PhaseOrdering/minmax.ll
23

Should be helped by:
01ae6e5ead64
...but still need to adapt at least 1 more fold and/or analysis to recognize intrinsics.