Update comment

Remove upper limit of rank altogether.

Update index calculation yet again. Some tests failed - use stride for index calculation
and divide by itemsize at the end.

Fix yet another small-vector issue.

Update to fix pre-commit build - Small Vector needs size.

Updates in this revision:

• Don't start with a zero and add/multiply that into the index calculation.
• Don't ask for the Nth dims in N-dimensional array (unless N = 1)
• Fix up tests to match the updated code-gen.
• Remove hard-coded value in tests that shouldn't be checked.

In D150687#4346761, @tianshilei1992 wrote:

Hmm, I'm not sure if we want to include <cmath> here because libomp definitely doesn't depend on libm. We might want to deal with it in https://reviews.llvm.org/D149010 instead.

Update docs to let people know we changed to zero initialize some
globals.

Just create a ZeroOp instead of calling createNullConstant.

In D149877#4321929, @jeanPerier wrote:

Thanks for working on this!

Note that there is a fir::ZeroOp that is intended to create some zero initialized data regardless of the type, but it currently is a TODO in codegen for aggregate/array types (That is actually the fir.zero_init). I think a zeroinitialzer operation may be required in LLVM MLIR dialect (like for the poison/undef op), so that we can propagate fir::ZeroOp into an LLVM zeroinitializer.

Remove empty lines

Rebase only

Review comment changes:

• Add checks for "no result" from the operations
• Fix code generated for array initializers
• Change the names of op2 -> op, and opsOfInterest -> loopsOfInterest
• Some typos in comments fixed

Rebase only

Fix unnecessary curly braces

Remove unnecessary braces in if-statement.

Rebase only

• Add new function to help clarify the purpose of the options being used.
• Add descriptive comment to make function use clearer.
• Update tests to make them a more explicit.

• Review comment updates. All are comment wording and formatting, no actual code changes

Rebase only.

Add size to SmallVector to allow Windows build.

Update moved header file and include kindmapping that we rely on.

• Change wording in several comments.
• Change one "auto" to real type.
• Rename misspelled filename and add newline at the end of the file

Rebase only

In D141306#4243135, @Leporacanthicus wrote:

In D141306#4218704, @peixin wrote:

The loop versioning is inefficient for the following scenario, in which not all arrays are contiguous in a loop. Maybe there should be one cost model to perform the loop versioning, and be refined driven by the cases in real workloads?

<snip code sample>

In D141306#4218704, @peixin wrote:

The loop versioning is inefficient for the following scenario, in which not all arrays are contiguous in a loop. Maybe there should be one cost model to perform the loop versioning, and be refined driven by the cases in real workloads?

subroutine vadd(c, a, b, n)
  implicit none
  real :: c(:), a(:), b(:)
  integer :: i, n

  print *, is_contiguous(b)  ! classic-flang is wrong here
  do i = 1, n
    c(i) = 5. * a(i) + 3. * b(i)
    c(i) = c(i) * a(i) + a(i) * 4.
  end do
end

interface
  subroutine vadd(c, a, b, n)
    implicit none
    integer :: n
    real :: c(:), a(:), b(:)
  end
end interface

  integer, parameter :: ub = 10
  real :: a(ub) = (/ (i, i=1, 10) /)
  real :: b(ub*2) = (/ (i, i=1, 20) /)
  real :: c(ub) = 0.
  call vadd(c(1:ub), a(1:ub), b(1:ub:2), 10)
  print *, c
end

BTW, classic-flang use copy-in-copy-out in caller for assumed-shape array, and all the strides are removed in the callee.

• Rebase
• Fix review comments

Rebase.

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.