[PATCH] D73064: [LoopCacheAnalysis]: Add support for negative stride

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

In D73064#1852211 <https://reviews.llvm.org/D73064#1852211>, @rcraik wrote:

> I took each of these example loops and, taking `N = 1024`, ran each loop 10 000 000 times. These testcases I ran 10 times each and found no statistical difference between either the number of L1 <https://reviews.llvm.org/L1> cache load misses or the L1 <https://reviews.llvm.org/L1> cache miss rate between each testcase. This indicates that the cost of these loops is the same, which is what this patch implements.


Thanks for trying to verify this but there is a problem with the setup. With `N = 1024`, the memory footprint `M` of each example loops is `1024 * sizeof(A[0])`, now I don't know what types you used but let's assume `double`. The footprint `M` is `1024 * sizeof(double) = 1024 * 8 = 8192` bytes. My laptop L1 <https://reviews.llvm.org/L1> data cache is 32KiB in size. That means I can fit the array 3 times into the L1 <https://reviews.llvm.org/L1> cache. Now, once the loop was traversed a single time, the L1 <https://reviews.llvm.org/L1> cache contains all of the array and there is little reason to assume eviction. Every consequent iteration, so `10 000 000 -1` times, will most likely only see L1 <https://reviews.llvm.org/L1> cache hits no matter how you iterate over the array.



================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:359
 
+      // The array may be accessed in reverse, for example:
+      //
----------------
jdoerfert wrote:
> The example code is not really helpful here.
Especially since it is way to complex. Why do we need the struct, the function, ...


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:381
+                                    AccessFnAR->getNoWrapFlags());
+      }
       const SCEV *Div = SE.getUDivExactExpr(AccessFn, ElemSize);
----------------
rcraik wrote:
> jdoerfert wrote:
> > No braces.
> I checked the LLVM Coding Standards, and it doesn't mention braces vs no braces for single statement if/for/etc. blocks. Since this statement covers multiple lines, I would rather keep the braces for clarity.
FWIW, I went through the coding standard myself again. As far as I can tell, every conditional follows the same rule: If it's a single statement, no braces, otherwise braces. (the alternative case has braces if the consequence has and vise versa)


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

https://reviews.llvm.org/D73064