[PATCH] D137019: [InstCombine] refactor the SimplifyUsingDistributiveLaws NFC

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:636
 /// (e. g. "(A*B)+(A*C)" -> "A*(B+C)").
-Value *InstCombinerImpl::tryFactorization(BinaryOperator &I,
-                                          Instruction::BinaryOps InnerOpcode,
-                                          Value *A, Value *B, Value *C,
-                                          Value *D) {
+Value *tryFactorization(BinaryOperator &I, const SimplifyQuery &SQ,
+                        InstCombiner::BuilderTy &Builder,
----------------
Make this static?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:733
 
+Value *InstCombinerImpl::SimplifyFactorization(BinaryOperator &I) {
+  Value *LHS = I.getOperand(0), *RHS = I.getOperand(1);
----------------
The current formatting rules say function names begin with a lower-case letter.
I'd prefer not to use the term "Simplify" here because that implies that we are not creating new values (InstSimplify). 
Use "doFactorization()" or "tryFactorizationFolds" or some variant of that?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:776
 /// Returns the simplified value, or null if it didn't simplify.
 Value *InstCombinerImpl::SimplifyUsingDistributiveLaws(BinaryOperator &I) {
   Value *LHS = I.getOperand(0), *RHS = I.getOperand(1);
----------------
Since we're making changes here, I'd rename this to meet current formatting guidelines.
"foldUsingDistributiveLaws()" ?


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

https://reviews.llvm.org/D137019