This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] push constant operand up/inside in sequence of min/max intrinsics
AbandonedPublic

Authored by spatel on Feb 15 2022, 8:31 AM.

Details

Summary

A generalization like this was suggested in D119754, but I'm proposing to move the constant up rather than down.

If these operations ever graduate to become real instructions rather than intrinsics, then this would not conflict with the reassociate pass because it has the same transform in its set of tricks for binop instructions.

The line between the passes on these kinds of folds is blurry, but this doesn't appear to have much cost and gives us the expected wins from repeated folds as seen in the last pair of test diffs.

Diff Detail

Event Timeline

spatel created this revision.Feb 15 2022, 8:31 AM
spatel requested review of this revision.Feb 15 2022, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2022, 8:31 AM
nikic added inline comments.Feb 16 2022, 12:36 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
897

I'd move the first check into the first if, because it doesn't match the comment on this one.

905

C1 -> C now

llvm/test/Transforms/InstCombine/minmax-intrinsics.ll
2268

What would happen if you swapped the operands on this smax? Unless I'm missing something, in the case of non-constant arguments, we're going to make a pretty arbitrary reassociation choice, which will depend on non-canonicalized argument order to produce a good result.

spatel marked 3 inline comments as done.Feb 16 2022, 6:11 AM
spatel added inline comments.
llvm/test/Transforms/InstCombine/minmax-intrinsics.ll
2268

Nice catch! I'll add a test.

We don't do the more elaborate complexity-based canonicalization for intrinsics that we do for commutative instructions, so the operand order on the middle max is not fixed.

We could pick another matching intrinsic if it exists, but there's no complete solution within the scope of instcombine AFAIK.

lebedev.ri added inline comments.Feb 16 2022, 6:13 AM
llvm/test/Transforms/InstCombine/minmax-intrinsics.ll
2268

That is one of the reasons why i'm suggesting sinking :)

spatel updated this revision to Diff 409228.Feb 16 2022, 6:21 AM
spatel marked an inline comment as done.

Patch updated:

  1. Improved order of matching.
  2. Fixed code comment.
  3. Added TODO comment/test for possible enhancement.
spatel marked an inline comment as done.Feb 16 2022, 8:35 AM
spatel added inline comments.
llvm/test/Transforms/InstCombine/minmax-intrinsics.ll
2268

Yes, that does seem better. I don't see any real downside other than inverting the theoretical transform from -reassociate.

Maybe we can justify that because we increase the chance of finding the optimal sequence by trying both directions. :)

spatel marked an inline comment as done.Feb 16 2022, 9:20 AM

Patch to go in the other direction:
D119955

spatel abandoned this revision.Feb 17 2022, 8:03 AM

Abandoning - pushed the transform in the other direction with 58df2da0540c