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

[Wei Mi via llvm-commits]

wmi added a comment.

Thanks for the 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)
----------------
qcolombet wrote:
> While updating the canonical representation, please make sure to update the comment on Formula::BaseRegs.
Ok, I will add the comment.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:389
+  if (Scale != 0 && Scale != 1)
+    return true;
+
----------------
qcolombet wrote:
> Shouldn't this be just Scale != 1.
> I don't expect we would generate 0 scaled register.
> What am I missing?
You are right. Scale will not equal to 0 when ScaledReg != NULL.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1273
-
   if (!Formulae.empty() && RigidFormula)
     return false;
----------------
qcolombet wrote:
> Why are we dropping this assert?
> That doesn't sound right.
I thought it was guaranteed to be true if we did canonicalization before inserting any formula. But as your comments below, we havn't agreed to do implicit canonicalization.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:3094
+
+  F.canonicalize(*L);
   if (!LU.InsertFormula(F))
----------------
qcolombet wrote:
> 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.
> 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.

It is reasonable. I will figure out where the canonicalization is needed now, and see how it looks like. 


Repository:
  rL LLVM

https://reviews.llvm.org/D26781