[PATCH] D22341: MinMax Index Pattern Identification

[Elena Demikhovsky via llvm-commits]

delena added inline comments.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:162
@@ +161,3 @@
+  static bool isMinMaxIndexOperands(ScalarEvolution *SE, Value *InductionCandidate,
+                                                 Value *OldValueCandidate, Value *Result);
+
----------------
line alignment

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:206
@@ +205,3 @@
+  void setCmpInstr(Instruction *Instr) { 
+    assert(((Kind == RK_IntegerMinMax) || (Kind == RK_FloatMinMax)) &&
+           "Should be MinMax Recurrence");
----------------
You don't need to check the Kind here - it is just get/set.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:387
@@ +386,3 @@
+      (RD.getRecurrenceKind() == RK_FloatMinMax)) && MinMaxIndexUseFound ) {
+    if (MinMaxSharedCmpInstr) {
+      RD.setCmpInstr(MinMaxSharedCmpInstr);
----------------
remove {}

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:424
@@ +423,3 @@
+  return isMinMaxIndexOperands(SE, SI->getTrueValue(), SI->getFalseValue(), SI) ||
+           isMinMaxIndexOperands(SE, SI->getFalseValue(), SI->getTrueValue(), SI);
+}
----------------
line alignment

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:537
@@ -452,1 +536,3 @@
   case Instruction::Select:
+    if (I->getOpcode() == Instruction::Select &&
+        Kind == RK_IntegerMinMaxIdx) {
----------------
The code here is difficult to read. I suggest to separate Select form FCmp and ICmp

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:540
@@ +539,3 @@
+      // We have a minmax index candidate
+      if (isMinMaxIndexCandidate(SE, dyn_cast<SelectInst>(I))) {
+        return InstDesc(true, I);
----------------
remove {}

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:600
@@ +599,3 @@
+    DEBUG(dbgs() << "Suspect a MINMAX INDEX reduction PHI." << *Phi << "\n");
+    return true; //TODO: FIX
+  }
----------------
What FIX should be done here?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4896
@@ +4895,3 @@
+          // the Phi's
+          if (RedDes.getRecurrenceKind() == RecurrenceDescriptor::RK_IntegerMinMaxIdx) {
+            ExtraVerificationRequired[Phi] = RedDes;
----------------
Why FP min/max does not require extra verification?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4932
@@ -4918,1 +4931,3 @@
 
+      // We have Reductions that need extra verfication phase (depends on other
+      // reductions results)  
----------------
I suggest to take this verification in a separate function.


Repository:
  rL LLVM

https://reviews.llvm.org/D22341