Page MenuHomePhabricator

[InstCombine] Canonicalize SPF to min/max intrinsics
ClosedPublic

Authored by nikic on Mar 7 2021, 1:21 PM.

Details

Summary

Now that min/max intrinsics have good support in both InstCombine and other passes, start canonicalizing SPF min/max to intrinsic min/max.

Once this sticks, we can stop matching SPF min/max in various places, and can remove hacks we have for preventing infinite loops / breaking SPF canonicalization.

Diff Detail

Unit TestsFailed

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@nikic What do you think is left to do before taking this forward for general review?

prof metadata support for abs/min/max is still missing.

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

Regression.

spatel added inline comments.Sep 26 2021, 6:53 AM
llvm/test/Transforms/InstCombine/max-of-nots.ll
351

There's probably still at least one gap in the matching for this kind of larger pattern, but we should get all of the existing tests after:
28afaed691a0a7ca

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

This should make it better:
6063e6b499c7829b

xbolva00 added inline comments.Sep 26 2021, 7:18 AM
llvm/test/Transforms/InstCombine/select_meta.ll
163–165

Regression.

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

Not done?

155

Regression

Matt added a subscriber: Matt.Oct 2 2021, 6:43 AM

@nikic @spatel Where have we gotten to with this? Is now the time to get it finished in plenty of time for 15.x ?

A local rebase indicates that after rGaca355a3bb99 the most obvious regressions have been squashed

llvm/test/Transforms/InstCombine/select-pr39595.ll
8

We lose pgo data here - I'm not sure whether this is an issue or not - PR39595 had branch weight metadata but it doesn't appear to have been critical to the bug.

@nikic @spatel Where have we gotten to with this? Is now the time to get it finished in plenty of time for 15.x ?

I think we're close. Even if there are some regressions still visible, the benefits should outweigh those? We could file issues for whatever is known left to do based on a rebased patch.

nikic added inline comments.Feb 14 2022, 8:22 AM
llvm/test/Transforms/InstCombine/minmax-fold.ll
283

This looks like a valuable fold.

spatel added inline comments.Feb 14 2022, 8:36 AM
llvm/test/Transforms/InstCombine/minmax-fold.ll
283

Agree - I'll work on that one.
There should be some family of patterns like this:
https://alive2.llvm.org/ce/z/8nsAMp

spatel added inline comments.Feb 14 2022, 8:41 AM
llvm/test/Transforms/InstCombine/minmax-fold.ll
283

On 2nd thought, this is just a reassociation/simplify fold when we have 2 constant operands and matching min/max:
https://alive2.llvm.org/ce/z/wW5HVM

RKSimon added inline comments.Feb 14 2022, 9:00 AM
llvm/test/Transforms/InstCombine/minmax-fold.ll
283

Is a check for constants enough here or do we need full value tracking analysis?

spatel added inline comments.Feb 14 2022, 11:19 AM
llvm/test/Transforms/InstCombine/minmax-fold.ll
283

A general check/fold for constants would already be an improvement over the corresponding fold for select patterns, so I'd start with that alone. I'm not seeing any regression test evidence that we need to try harder so far.

RKSimon added inline comments.Feb 16 2022, 5:05 AM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
1476

NewAbs -> NewSPF

nikic updated this revision to Diff 409222.Feb 16 2022, 5:50 AM
nikic marked an inline comment as done.

Rebase

nikic added a comment.Feb 23 2022, 2:36 AM

I just went through all the tests again, and these are the remaining regressions I spotted. (There are some differences in canonicalization, but these are the ones where it's clearly worse now.)

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

This regression still exists.

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

Regression still exists.

78

Regression.

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

Still regresses.

nikic updated this revision to Diff 410811.Feb 23 2022, 6:59 AM
nikic edited the summary of this revision. (Show Details)
nikic added reviewers: spatel, RKSimon, lebedev.ri.

Rebase. The three remaining regressions are fixed now.

spatel accepted this revision.Feb 23 2022, 7:51 AM

Rebase. The three remaining regressions are fixed now.

That was fast! I was going to look at the remaining bits, but too late. :)

Thank you for pushing it forward. LGTM.

For reference, these were at least some of the commits to handle the last round of missing folds:
e2f627e5e385
6777ec9e4df7
5ccb0582c2b1
03e6efb8c23f
587c7ff15c26

This revision is now accepted and ready to land.Feb 23 2022, 7:51 AM
This revision was landed with ongoing or failed builds.Feb 24 2022, 12:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2022, 12:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nlopes added a subscriber: nlopes.Feb 25 2022, 2:13 AM

This fixed 9 bugs reports by Alive2:

  • Transforms/InstCombine/max-of-nots.ll
  • Transforms/InstCombine/max_known_bits.ll
  • Transforms/InstCombine/minmax-fold.ll
  • Transforms/InstCombine/saturating-add-sub.ll
  • Transforms/InstCombine/select-pr39595.ll
  • Transforms/InstCombine/select_meta.ll
  • Transforms/InstCombine/sext.ll
  • Transforms/InstCombine/sub.ll
  • Transforms/InstCombine/with_overflow.ll

Thank you!

Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2022, 11:20 PM