Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flang] verification of lowering to FIR failed with "'hlfir.minval' op result must have the same element type as ARRAY argument" #79995

Closed
k-arrows opened this issue Jan 30, 2024 · 5 comments · Fixed by #80132
Labels

Comments

@k-arrows
Copy link

Reproducible on Godbolt:
https://godbolt.org/z/T5MeEa7r3

Reproducer:

    character(3) :: char(3, 4)
    char = 'abc'

    associate (y=>func(char))
        associate (item => y(1:2)//'aa')
        end associate
    end associate

    contains

    function func(char)
       character(:), allocatable :: func(:)
       character(3) :: char(3, 4)

       func = minval(char//' ', dim=1)
    end function
end

If I use -flang-deprecated-no-hlfir option, the following TODO message is obtained.

error: loc("/app/example.f90":15:8): /root/llvm-project/flang/lib/Lower/ConvertExpr.cpp:3412: not yet implemented: gather rhs LEN parameters in assignment to allocatable

https://godbolt.org/z/Enj4Gcajh

@github-actions github-actions bot added the flang Flang issues not falling into any other category label Jan 30, 2024
@EugeneZelenko EugeneZelenko added flang:ir and removed flang Flang issues not falling into any other category labels Jan 30, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 30, 2024

@llvm/issue-subscribers-flang-ir

Author: None (k-arrows)

Reproducible on Godbolt: https://godbolt.org/z/T5MeEa7r3

Reproducer:

    character(3) :: char(3, 4)
    char = 'abc'

    associate (y=>func(char))
        associate (item => y(1:2)//'aa')
        end associate
    end associate

    contains

    function func(char)
       character(:), allocatable :: func(:)
       character(3) :: char(3, 4)

       func = minval(char//' ', dim=1)
    end function
end

If I use -flang-deprecated-no-hlfir option, the following TODO message is obtained.

error: loc("/app/example.f90":15:8): /root/llvm-project/flang/lib/Lower/ConvertExpr.cpp:3412: not yet implemented: gather rhs LEN parameters in assignment to allocatable

https://godbolt.org/z/Enj4Gcajh

@tblah
Copy link
Contributor

tblah commented Jan 30, 2024

F2018 16.9.134 4 says that the result of minval has the "same type and type parameters as ARRAY". So the op verifier is right to expect these to have the same type.

Looking at the generated HLFIR, char is a !fir.ref<!fir.array<3x4x!fir.char<1,3>>>, whereas func is fir.ref<!fir.box<!fir.heap<!fir.array<?x!fir.char<1,?>>>>>.

The check is here:

I believe this is too strict, and needs to be updated to understand that an unknown type parameter can be compatible with a known parameter (in this case fir.char<1,?> is compatible with !fir.char<1,3>).

Is this solution what you would expect?

@jeanPerier
Copy link
Contributor

lieve this is too strict, and needs to be updated to understand that an unknown type parameter can be compatible with a known parameter (in this case fir.char<1,?> is compatible with !fir.char<1,3>).

Is this solution what you would expect?

I agree. In general, I am also a bit worried about FIR/HLFIR operations being too nitpicky about the type extents/lengths in checks. The rational being that inlining may could in the future cause some of the operands type to change and be invalid even if the code as such is not reachable.

Think of:

subroutine foo(c)
 character(*) :: c(10)
 if (len(c) == 3) then
   call some_specialized_version_for_length_3(c)
  else
   call generic_version(c)
 end if
end subroutine

subroutine some_specialized_version_for_length_3(c)
  character(3) :: c(10)
  print *, minval(c)
end subroutine

 character(5) :: c(10)
 call foo(c)
end

If all the functions get inlined and types are propagated, and for some reasons the branch containing the minval call is not pruned, the compiler would throw an internal compiler error for unreachable code that it generated.

This is a lot of ifs, so it is likely OK to keep it as it is, but I would say MLIR verifiers are needed to enforce guarantees that would lead to compiler crash if violated, but that we should rely on semantics/runtime checks for the Fortran constraints that would not prevent the IR to be generated/translated when violated.

@k-arrows
Copy link
Author

Thank you for your clarification. I also think the verifier is too strict here. I've tried a few other compilers and none of them complained about this code. But I haven't encountered this problem in a real application, so it doesn't really bother me.

tblah added a commit to tblah/llvm-project that referenced this issue Jan 31, 2024
The verifiers are currently very strict: requiring intrinsic operations
to be used only in cases where the Fortran standard permits the
intrinsic to be used.

There have now been a lot of cases where these verifiers have caused
bugs in corner cases. In a recent ticket, @jeanPerier pointed out that
it could be useful for future optimizations if somewhat invalid uses of
these operations could be allowed in dead code. See this comment:
llvm#79995 (comment)

In response to all of this, I have decided to relax the intrinsic
operation verifiers. The intention is now to only disallow operation
uses that are likely to crash the compiler. It isn't obvious which
checks are more useful than others, and it would not be a good use of time
to try lots of illegal (according to Fortran) intrinsic uses to figure out
exactly which can crash Flang (as none of these should pass semantics
anyway). I have taken an educated guess.

The disadvantage of this approach is that IR can now represent intrinsic
invocations which are incorrect. The lowering and implementation of
these intrinsic functions is unlikely to do the right thing in all of
these cases, and as they should mostly be impossible to generate using
normal Fortran code, these edge cases will see very little testing,
before some new optimization causes them to become more common.

Fixes llvm#79995
@tblah
Copy link
Contributor

tblah commented Jan 31, 2024

Thanks for the example Jean, that makes a lot of sense. I've made a patch to relax the verifiers.

tblah added a commit that referenced this issue Feb 1, 2024
The verifiers are currently very strict: requiring intrinsic operations
to be used only in cases where the Fortran standard permits the
intrinsic to be used.

There have now been a lot of cases where these verifiers have caused
bugs in corner cases. In a recent ticket, @jeanPerier pointed out that
it could be useful for future optimizations if somewhat invalid uses of
these operations could be allowed in dead code. See this comment:
#79995 (comment)

In response to all of this, I have decided to relax the intrinsic
operation verifiers. The intention is now to only disallow operation
uses that are likely to crash the compiler. Other checks are still
available under `-strict-intrinsic-verifier`.

The disadvantage of this approach is that IR can now represent intrinsic
invocations which are incorrect. The lowering and implementation of
these intrinsic functions is unlikely to do the right thing in all of
these cases, and as they should mostly be impossible to generate using
normal Fortran code, these edge cases will see very little testing,
before some new optimization causes them to become more common.

Fixes #79995
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Feb 1, 2024
The verifiers are currently very strict: requiring intrinsic operations
to be used only in cases where the Fortran standard permits the
intrinsic to be used.

There have now been a lot of cases where these verifiers have caused
bugs in corner cases. In a recent ticket, @jeanPerier pointed out that
it could be useful for future optimizations if somewhat invalid uses of
these operations could be allowed in dead code. See this comment:
llvm#79995 (comment)

In response to all of this, I have decided to relax the intrinsic
operation verifiers. The intention is now to only disallow operation
uses that are likely to crash the compiler. Other checks are still
available under `-strict-intrinsic-verifier`.

The disadvantage of this approach is that IR can now represent intrinsic
invocations which are incorrect. The lowering and implementation of
these intrinsic functions is unlikely to do the right thing in all of
these cases, and as they should mostly be impossible to generate using
normal Fortran code, these edge cases will see very little testing,
before some new optimization causes them to become more common.

Fixes llvm#79995
agozillon pushed a commit to agozillon/llvm-project that referenced this issue Feb 5, 2024
The verifiers are currently very strict: requiring intrinsic operations
to be used only in cases where the Fortran standard permits the
intrinsic to be used.

There have now been a lot of cases where these verifiers have caused
bugs in corner cases. In a recent ticket, @jeanPerier pointed out that
it could be useful for future optimizations if somewhat invalid uses of
these operations could be allowed in dead code. See this comment:
llvm#79995 (comment)

In response to all of this, I have decided to relax the intrinsic
operation verifiers. The intention is now to only disallow operation
uses that are likely to crash the compiler. Other checks are still
available under `-strict-intrinsic-verifier`.

The disadvantage of this approach is that IR can now represent intrinsic
invocations which are incorrect. The lowering and implementation of
these intrinsic functions is unlikely to do the right thing in all of
these cases, and as they should mostly be impossible to generate using
normal Fortran code, these edge cases will see very little testing,
before some new optimization causes them to become more common.

Fixes llvm#79995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants