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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:319
+      return dyn_cast_or_null<Instruction>(
+          tryReassociateMinOrMax(I, SMaxMatch, LHS, RHS));
+    }
----------------
Can we factor out code of `if(match) { ... }` into a lambda that takes a matcher and returns instruction and avoid this copy-paste?


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:606
+template <typename MaxMinT>
+Value *NaryReassociatePass::tryReassociateMinOrMax(Instruction *I,
+                                                   MaxMinT MaxMinMatch,
----------------
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?


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:635
+
+        Value *R1MinMax = findClosestMatchingDominator(R1Expr, I);
+
----------------
`findClosestMatchingDominator` returns an `Instruction *`, do we really need to downcast it do `Value *`


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:649
+        Value *NewMinMax = Expander.expandCodeFor(R2Expr, I->getType(), I);
+        NewMinMax->setName(Twine(I->getName()).concat(".nary"));
+
----------------
Does it make more sense to name it `".reassociate"` instead?


================
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
----------------
Please commit these tests with auto-generated checks without your patch and rebase on top of it to see what your patch changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88287