In D141306#4201153, @peixin wrote:

I failed to apply this patch locally.

Rebase only

Rebase and fix conflicts

In D141306#4201153, @peixin wrote:

I failed to apply this patch locally.

In D141306#4198175, @peixin wrote:

Yes, I've only spent SOME time trying to understand why vectorizer doesn't - it basically comes down to "can't figure out how the stride is calculated, and whether it may change over time". The loop versioning here is helping to solve that for the lower layers in the compiler - and in my experience (I've written quite a few different style loops and such), either some MLIR pass(es) manages to vectorize the codr, or LLVM loop vectorizer can't do it either. [Why is there two layers of vectorizers? I don't know - presumably people like to write code that does similar things on multiple levels].

OK.

Also, the overall goal isn't ONLY to vectorize, but also to "improve overall loop performance". It may be that we can convince the compiler to do a better job in some other way, but right now, I'm not aware of what that should be. The fundamental problem is that the descriptors used to pass array information is getting in the way of lots of things. It should be noted that although we see performance improvement in roms_r, it does NOT come from vectorization of the loop constructs - both gfortran and flang uses loop versioning, and gets similar performance improvement - and neither compiler uses vector instructions in the hot loops in ROMS.

I am confused about how the performance gets improved for roms_r you mentioned. From the test in this patch, I can only see the optimization opportunity of loop vectorization after loop versioning. Maybe another test case extracted in roms_r showing how it gets performance improvement is better.

In D141306#4198192, @peixin wrote:

FYI, another related scenario is https://github.com/llvm/llvm-project/issues/59388.

I also faced one case having loop interchange opportunity, which is blocked by the Fortran array stride information not understood by SCEV analysis. I think this is one common issue in loop optimization. So there maybe need some discussions whether loop versioning in MLIR pass is the right solution.

LGTM.

In D141306#4195254, @peixin wrote:

I have not noticed any measurable increase in compile time - Spec 2017 wrf_r takes within 1-2 seconds, and that's the one that takes about 15 minutes to build in total. I'm not saying it's impossible to come up with something that compiles slowly with this code, but I've made as good an attempt as I can to "exit early" if there's nothing that needs doing, and only duplicate the innermost loop [which potentially is not the most optimal case].

Great! This sounds good to me.

The problem I'm trying to solve here is that the Loop Vectorizer optimisation doesn't "like" the descriptor access to read the strides [and other descriptor accesses] - it doesn't understand that the value is a constant for a given run of a loop. With versioning , it basically makes the Loop Vectorizer "happy" with the loop, so it can do its own thing.

I think the loop vectorize pass is doing the similar thing. Check the generated LLVM IR the following Fortran case:

subroutine sum1d(a, n)
  real*8 :: a(:)
  integer :: n
  real*8 :: sum
  integer :: i
  sum = 0
  do i=1,n
     sum = sum + a(i)
  end do
  call temp(sum)
end subroutine sum1d

$ flang-new -fc1 -emit-llvm -O3 test.f90
$ cat test.ll
define void @sum1d_(ptr nocapture readonly %0, ptr nocapture readonly %1) local_unnamed_addr {
  %3 = alloca double, align 8
  store double 0.000000e+00, ptr %3, align 8, !tbaa !1
  %4 = load i32, ptr %1, align 4, !tbaa !1
  %5 = icmp sgt i32 %4, 0
  br i1 %5, label %.lr.ph, label %._crit_edge

.lr.ph:                                           ; preds = %2
  %6 = zext i32 %4 to i64
  %7 = load ptr, ptr %0, align 8, !tbaa !5
  %8 = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %0, i64 0, i32 7, i64 0, i64 2
  %9 = load i64, ptr %8, align 8, !tbaa !5
  br label %10

10:                                               ; preds = %.lr.ph, %10
  %indvars.iv = phi i64 [ 1, %.lr.ph ], [ %indvars.iv.next, %10 ]
  %11 = phi double [ 0.000000e+00, %.lr.ph ], [ %16, %10 ]
  %12 = add nsw i64 %indvars.iv, -1
  %13 = mul i64 %9, %12
  %14 = getelementptr i8, ptr %7, i64 %13                                                            ! Note: This has the similar structure as the LLVM IR generated from the following C code.
  %15 = load double, ptr %14, align 8, !tbaa !1
  %16 = fadd contract double %11, %15
  store double %16, ptr %3, align 8, !tbaa !1
  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
  %exitcond.not = icmp eq i64 %indvars.iv, %6
  br i1 %exitcond.not, label %._crit_edge, label %10

._crit_edge:                                      ; preds = %10, %2
  call void @temp_(ptr nonnull %3)
  ret void
}

When you compare it with the generated IR from the following C case using clang case.c -S -emit-llvm -O3:

void temp(double *sum);

void test1(double * restrict A, double * restrict B, int N, int Stride) {
  int i;
  for (i = 0; i < N; ++i)
    B[i * Stride] += A[i * Stride];
}

void test2(double * restrict A, int N, int Stride) {
  int i;
  double sum = 0.0;
  for (i = 0; i < N; ++i)
    sum += A[i * Stride];
  temp(&sum);
}

The loop in function test1 can be vectorized using stride-check loop versioning. I don't dig why test2 fail in vectorizing the reduction. Anyway, the loop vectorize pass can do some similar work. But I am not sure if the SCEV analysis there can handle the Fortran case.

In D141306#4192692, @peixin wrote:

I am thinking if we should do the loop versioning in MLIR since this may increase compilation time in optimization passes.

Updated to reflect review comments.

• Use different type for Walk functions.
• Use "unwrap" functions to get to types.
• Fix some debug output and related stuff.
• Some other minor changes, including clang-format fixups.

Fix copied comment to reflect the new content.

Updates based on review comments:

• Add tests.
• Enable on -O3

Add tests.

Update for rebase.

LGTM

Generally looks OK, just some minor nitpicks.

Update to remove WIP

Rebased and updated help-message

Aside from minor nit-picks, this looks good to me.

LGTM.

Updated patch, now works for 1D and 2D arrays.

Some progress, now identifying the correct loops a little better.

LGTM, thanks for fixing!

Looks OK except for the test checking for the wrong "Not".

I think this is good to go in now, subject to @vzakhari not finding anything else wrong.

Looks OK in general, just some minor tidy-up suggestions.

Rebase again

Rebase

Rebase to latest llvm/main.

Added "move remaining loop into else branch" and related changes.

In D141306#4037682, @vzakhari wrote:

Hi @Leporacanthicus, thank you for working on this! I wonder if you target any particular benchmark with these changes - can you please post any information in the description?

This is work described here:
https://discourse.llvm.org/t/transformations-to-aid-optimizer-for-subroutines-functions-with-assumed-shape-arguments/66447/3

Try again, this time without requires line

In D137335#3905159, @awarzynski wrote:

What if you remove --target=x86_64-unknown-linux-gnu instead and make the tests target agnostic?

Instead of requiring x86, remove target in tests. This will allow
testing on a wider range of targets, rather than a smaller range.

Update to correct typos.

Update comments (clarify/reword, fix typos) and rebase.

Updates per review comments and local testing.

@MatsPetersson & @clementval , could you share you build command so that the failure can be reproduced before this re-lands?

I can't share the same command that I used, because it's some old CMAKE command that I no longer have in my history. But I tested this patch, and it works in the same tree that I was using it last week.

LGTM.

Updates based on review comments:

• Additional MLIR DataLayout.
• Add check lines for datalayout in the MLIR and LLVM output

Again, thanks for the work here. It looks roughly like I had imagined [although I was kind of wishfully thinking that maybe we could just treat ND as 1D, just larger - at least for things where "ordering isn't important", such as sum or maxval]

Thanks for working on this!

LGTM

In D132652#3764143, @vzakhari wrote:

LGTM, except for the buildbot failures.

Rebase to latest llvm/main

Added const to KindMapping argument.

Revew comment fixes:

• Removed unused header file
• Changed function names from RTName to Runtime to better reflect what they do.

Updates per review comments:

• Fix typos
• Use llvm::function_ref instead of std::function

Updated comment for helper function

Trying again, patch seems to have got messed up.

I think my patch is messed up, however. It seems to have included my refactoring as well, which should be a separate patch.

Remove comment that was no longer valid

Review updates:

• Use different method to find a smallest value to start for MAXVAL
• Tidy up some code in general.
• Add descriptive comment to big function.
• Pass loc around in a few places.

LGTM! Thanks for the work on this!

LGTM. I do expect this to be short-lived [or at least for the SUM operations!]

Updated check for expected operation (always fir::ConvertOp) instead of
checking for number of operands. Assert to capture should number of operands
be zero.