[PATCH] D108112: [LoopIdiom] let LIR fold memset pointer / stride SCEV regarding loop guards

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

In D108112#2958737 <https://reviews.llvm.org/D108112#2958737>, @eopXD wrote:

> In D108112#2957417 <https://reviews.llvm.org/D108112#2957417>, @bmahjour wrote:
>
>> 
>
>
>
>> or if 'n' and 'm' where 'signed long long'?
>
> The accepted scenario for LIR is `MemsetSizeSCEV == PositivePointerStrideSCEV`.
> The `n, m` accepted would always be non-negative. I can't think of scenario when `n`, `m` needs to remain as a `signed long long`. (Please correct me if I'm wrong.

Since we don't normalize loops we may have reverse loops where the upper bound is a negative signed value. Another example is if IVs start with negative values:

  void foo(int n, int m, float Arr[n][m]) {
    for (int i = -100; i < n; i++)
      for (int j = -200; j < m; j++)
        Arr[i+100][j+200] = 1.0;
  }



>> I still have some reservations about the SCEVFolder implementation in this patch. It doesn't really fold anything, it just zero extends an expression if it is safe to do so. The assumption being made is that the expression it compares against is unsigned (so a "folding" opportunity is realized). In the LIT test provided the trip count is zero extended by loop simplify because it is originally 32-bit and the code is targeting 64-bit mode.
>
> I agree with you that `SCEVFolder` has trivial functionality. It only to turn `sext` to `zext`. This patch only fixes the test case added, which is very limited.

The term "fold" doesn't seem to be the right terminology. I think "rewrite" is more appropriate. For example, `SCEVFolder` may be called `SCEVSignToZeroExtentionRewriter` (or something similar).

> On the other hand, I don't think I have deep understanding to benchmarks. So I am not sure if I have covered most of the "practical memset idioms" that can be optimized here. Do you have other "fold loop-guard into SCEV-expression` cases that you wish this patch to have? I would be happy to resolve it.

I don't have any specific benchmarks in mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108112