Page MenuHomePhabricator

[NARY-REASSOCIATE] Support reassociation of min/max
Needs ReviewPublic

Authored by ebrevnov on Sep 25 2020, 1:56 AM.

Details

Summary

Support reassociation for min/max. With that we should be able to transform min(min(a, b), c) -> min(min(a, c), b) if min(a, c) is already available.

Diff Detail

Event Timeline

ebrevnov created this revision.Sep 25 2020, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 1:56 AM
ebrevnov requested review of this revision.Sep 25 2020, 1:56 AM
mkazantsev added inline comments.Thu, Oct 29, 9:54 PM
llvm/lib/Transforms/Scalar/NaryReassociate.cpp
319

Can we factor out code of if(match) { ... } into a lambda that takes a matcher and returns instruction and avoid this copy-paste?

606

The result of tryReassociateMinOrMax is only used as instruction, and non-instruction values are discarded by dyn_cast_or_null. Maybe change API to return instruction from here?

635

findClosestMatchingDominator returns an Instruction *, do we really need to downcast it do Value *

649

Does it make more sense to name it ".reassociate" instead?

llvm/test/Transforms/NaryReassociate/nary-smax.ll
1

Please commit these tests with auto-generated checks without your patch and rebase on top of it to see what your patch changes.

mkazantsev added inline comments.Thu, Oct 29, 9:56 PM
llvm/lib/Transforms/Scalar/NaryReassociate.cpp
606

Never mind, I think what you have is fine (I didn't notice it's a mutator).