[PATCH] D105009: [LSR] Handle case 1*reg => reg. PR50918

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:514
     return;
-  // So far we did not need this case. This is easy to implement but it is
-  // useless to maintain dead code. Beside it could hurt compile time.
----------------
mkazantsev wrote:
> reames wrote:
> > Given the comment indicates an unimplemented case here, why not just implement it?
> > 
> > Adding the following code just above the assert appears to address your test case.
> > 
> > +
> > +  if (BaseRegs.empty() && ScaledReg && Scale == 1) {
> > +    BaseRegs.push_back(ScaledReg);
> > +    Scale = 0;
> > +    ScaledReg = nullptr;
> > +    return;
> > +  }
> > 
> > Any reason why this is a poor approach?
> It seems that null scaled reg is not allowed, because this code always sets it to non-null.
> ```
>   if (!ScaledReg) {
>     ScaledReg = BaseRegs.pop_back_val();
>     Scale = 1;
>   }
> ```
> I don't know if there are other places that rely on this.
This comment is incorrect.  ScaledReg can be null, see isCanonical and the return just above this line.

The actual invariant appears to be that if ScaledReg is null, BaseRegs must have 0 or 1 element.  Which makes sense, because we're then describing either a constant, or register indexing mode.  


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

https://reviews.llvm.org/D105009