This is an archive of the discontinued LLVM Phabricator instance.

[flang] Fixed IsContiguous check for slices of parameter arrays.
ClosedPublic

Authored by vzakhari on Jul 27 2023, 3:59 PM.

Details

Summary

A section of a parameter array may be non-contiguous,
so the current !IsVariable(expr) check is too optimistic
to claim contiguity.

This patch fixes issues with incorrect hlfir.designate op generated
during lowering: the lowering queries IsContiguous to decide whether
to use fir.box<fir.array> or plain fir.ref<fir.array> to represent
the designator result.

Diff Detail

Event Timeline

vzakhari created this revision.Jul 27 2023, 3:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 3:59 PM
vzakhari requested review of this revision.Jul 27 2023, 3:59 PM
klausler accepted this revision.Jul 27 2023, 4:55 PM

Could you also please add a test to flang/test/Evaluate to check contiguity on named constants? That would be easier to debug if/when it breaks in the future. Just something like logical, parameter :: test_1 = is_contiguous(a(:,1)); logical, parameter :: test_2 = .not. is_contiguous(a(1,:)) would do it, I think.

This revision is now accepted and ready to land.Jul 27 2023, 4:55 PM

Could you also please add a test to flang/test/Evaluate to check contiguity on named constants? That would be easier to debug if/when it breaks in the future. Just something like logical, parameter :: test_1 = is_contiguous(a(:,1)); logical, parameter :: test_2 = .not. is_contiguous(a(1,:)) would do it, I think.

Thanks! Good idea. I tried adding this:

    integer, parameter :: cst(2,2) = reshape([1, 2, 3, 4], [2, 2])
    integer :: n
    logical, parameter :: test_param1 = is_contiguous(cst(:,1))
    logical, parameter :: test_param2 = is_contiguous(cst(1,:))
    logical, parameter :: test_param3 = is_contiguous(cst(:,n))
!    logical, parameter :: test_param4 = .not. is_contiguous(cst(n,:))

The last one does not work because of error: Must be a constant value for .not. is_contiguous(cst(n,:)). The test_param2 test looks okay to me, because of the constant folding. Do you have any suggestions how to make test_param4 check work?

Could you also please add a test to flang/test/Evaluate to check contiguity on named constants? That would be easier to debug if/when it breaks in the future. Just something like logical, parameter :: test_1 = is_contiguous(a(:,1)); logical, parameter :: test_2 = .not. is_contiguous(a(1,:)) would do it, I think.

Thanks! Good idea. I tried adding this:

    integer, parameter :: cst(2,2) = reshape([1, 2, 3, 4], [2, 2])
    integer :: n
    logical, parameter :: test_param1 = is_contiguous(cst(:,1))
    logical, parameter :: test_param2 = is_contiguous(cst(1,:))
    logical, parameter :: test_param3 = is_contiguous(cst(:,n))
!    logical, parameter :: test_param4 = .not. is_contiguous(cst(n,:))

The last one does not work because of error: Must be a constant value for .not. is_contiguous(cst(n,:)). The test_param2 test looks okay to me, because of the constant folding. Do you have any suggestions how to make test_param4 check work?

The internal implementation of IS_CONTIGUOUS returns a tristate std::optional<bool> that is populated only when the predicate is definitely true or false. It must have returned a std::nullopt here for test_param4, don't know why.

Ok, let me debug this a little bit more.

vzakhari updated this revision to Diff 545936.Aug 1 2023, 12:07 AM
  • Made response about array section non-contiguity more precise.
klausler added inline comments.Aug 1 2023, 11:18 AM
flang/lib/Evaluate/check-expression.cpp
870

You don't need to use GetPtrFromOptional here, just use a & reference: if (const auto &lb{baseLbounds->at(j)}) { ... }. Same with the upper bound.

flang/test/Evaluate/folding09.f90
20

Minor point: Instead of repeating the shape of the named constant as an argument to reshape, you can write shape(cst).

25

Should this be .not. is_contiguous(cst(1,:))?

vzakhari added inline comments.Aug 1 2023, 11:27 AM
flang/lib/Evaluate/check-expression.cpp
870

Thanks! I will do this.

flang/test/Evaluate/folding09.f90
20

Thanks!

25

is_contiguous returns true for this case. I think this happens because cst(1,:) is folded into a contiguous constant. This case is not hitting the CheckSubscripts code.

klausler added inline comments.Aug 1 2023, 11:34 AM
flang/test/Evaluate/folding09.f90
25

But then cst(n,:) should also be a folded contiguous constant, no?

I would recommend not using named constants for this test, just local arrays. Also, these "test_..." variables have to be top-level module variables for the magic in test_folding.py to see them.

vzakhari added inline comments.Aug 1 2023, 12:27 PM
flang/test/Evaluate/folding09.f90
25

cst(n,:) cannot be folded, because of unknown n. The initial problem is that is_contiguous(cst(n,:)) was reporting true.

I will move the variables to the top-level, but I would like to keep using named constants because this is what my changes are about.

klausler added inline comments.Aug 1 2023, 12:30 PM
flang/test/Evaluate/folding09.f90
25

Ok. It would be a good idea to compare your results with other compilers here.

One could make a case that is_contiguous(cst(n,:)) should be true, since cst(n,:) is not a variable.

klausler added a comment.EditedAug 1 2023, 12:40 PM

For

subroutine subr(n)
  integer, intent(in) :: n
  real, parameter :: c(2,2) = reshape([1.,2.,3.,4.],shape(c))
  print *, is_contiguous(c(:,1))
  print *, is_contiguous(c(1,:))
  print *, is_contiguous(c(:,n))
  print *, is_contiguous(c(n,:))
end

call subr(1)
end

all compilers agree that is_contiguous(c(:,1)) and is_contiguous(c(:,n)) are true, and that is_contiguous(c(n,:)) is false. They are inconsistent on the case of is_contiguous(c(1,:)).

I tried this case:

module m
  integer, parameter :: cst(2,2) = reshape([1, 2, 3, 4], [2, 2])
 contains
  subroutine test(n)
    integer :: n
    print *, is_contiguous(cst(n,:))
  end subroutine
end module

program main
  use m
  call test(1)
end program main

gfortran/ifx/ifort/xlf print F
nvfortran prints T

For

subroutine subr(n)
  integer, intent(in) :: n
  real, parameter :: c(2,2) = reshape([1.,2.,3.,4.],shape(c))
  print *, is_contiguous(c(:,1))
  print *, is_contiguous(c(1,:))
  print *, is_contiguous(c(:,n))
  print *, is_contiguous(c(n,:))
end

call subr(1)
end

all compilers agree that is_contiguous(c(:,1)) and is_contiguous(c(:,n)) are true, and that is_contiguous(c(n,:)) is false. They are inconsistent on the case of is_contiguous(c(1,:)).

Right, c(1,:) contiguity depends on whether it is represented with a separate constant object holding the copy of c(1,:) or it is represented as an array section of the original constant object c. I am not sure if it makes sense from the Fortran standard point of view, but it makes sense to me from the implementation point of view that chooses the final memory layout of the data.

vzakhari updated this revision to Diff 546190.Aug 1 2023, 12:53 PM
  • Addressed comments.