[PATCH] D26781: [LSR] Canonicalize formula and put recursive Reg related with current loop in ScaledReg.

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:384
 /// \see Formula::BaseRegs.
-bool Formula::isCanonical() const {
-  if (ScaledReg)
-    return Scale != 1 || !BaseRegs.empty();
-  return BaseRegs.size() <= 1;
+bool Formula::isCanonical(const Loop &L) const {
+  if (!ScaledReg)
----------------
While updating the canonical representation, please make sure to update the comment on Formula::BaseRegs.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:389
+  if (Scale != 0 && Scale != 1)
+    return true;
+
----------------
Shouldn't this be just Scale != 1.
I don't expect we would generate 0 scaled register.
What am I missing?


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1090
       // Otherwise, do not consider this formula at all.
-      Lose();
+      ++NumRegs;
       return;
----------------
This change shouldn't be part of the canonicalize patch IMO.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1273
-
   if (!Formulae.empty() && RigidFormula)
     return false;
----------------
Why are we dropping this assert?
That doesn't sound right.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1448
-  // compile time sake.
-  assert((F.isCanonical() || F.Scale != 0));
   return isAMCompletelyFolded(TTI, MinOffset, MaxOffset, Kind, AccessTy,
----------------
Ditto.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:3094
+
+  F.canonicalize(*L);
   if (!LU.InsertFormula(F))
----------------
So far I wanted the canonicalization process to be thoughtful, meaning, no implicit canonicalization like in this change.
The rationale is that in a lot of cases, we will try to canonicalize formulae that are canonical by construction, i.e., we waste compile time.
This is a trade-off between simplicity of the code and compile time.

Was it too complex to keep it that way?
Indeed, I wouldn't have expected we have to add calls to F.canonicalize, except where you changed something.

What am I saying is that unless the code becomes messy very quickly (or every InsertFormula call gets preceded by a call to canonicalize), I would rather keep it the way it was before.

Note: Aside from compile time, the main reason why I don't like that change is because the API looks strange now. I.e., F is not const anymore.


Repository:
  rL LLVM

https://reviews.llvm.org/D26781