[PATCH] D88287: [NARY-REASSOCIATE] Support reassociation of min/max

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:319
+      return dyn_cast_or_null<Instruction>(
+          tryReassociateMinOrMax(I, SMaxMatch, LHS, RHS));
+    }
----------------
mkazantsev wrote:
> Can we factor out code of `if(match) { ... }` into a lambda that takes a matcher and returns instruction and avoid this copy-paste?
Not sure how many lines of the code it will save.... let me try that... will see if it's any better


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:635
+
+        Value *R1MinMax = findClosestMatchingDominator(R1Expr, I);
+
----------------
mkazantsev wrote:
> `findClosestMatchingDominator` returns an `Instruction *`, do we really need to downcast it do `Value *`
I don't think it will make any difference in this particular case. No problems to change to Instruction*


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:649
+        Value *NewMinMax = Expander.expandCodeFor(R2Expr, I->getType(), I);
+        NewMinMax->setName(Twine(I->getName()).concat(".nary"));
+
----------------
mkazantsev wrote:
> Does it make more sense to name it `".reassociate"` instead?
In addition to NaryReassociate there is Ressociate pass. I want the name to clearly point to NaryReassociate pass. Using "reassociate" seems too long for me...


================
Comment at: llvm/test/Transforms/NaryReassociate/nary-smax.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -nary-reassociate -S | FileCheck %s
----------------
mkazantsev wrote:
> Please commit these tests with auto-generated checks without your patch and rebase on top of it to see what your patch changes.
Ok


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88287/new/

https://reviews.llvm.org/D88287