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.

In D144649#4178100, @clementval wrote:

FYI I see couple of gfortran tests related to isnan are failing

maxlocval_2
maxlocval_4
minlocval_1
minlocval_4
namelist_42
namelist_43
nan_6
nearest_3

Looks OK.

In D144896#4216634, @agozillon wrote:

In D144896#4216544, @kiranchandramohan wrote:

In D144896#4213015, @kiranchandramohan wrote:

In D144896#4212840, @agozillon wrote:

In D144896#4211723, @jdoerfert wrote:

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry for the delay here. I would like to just have a quick check whether having an interface is possible. If not, we can continue with this. I will come back to this tomorrow.

I did not get much time to spend on this. But I have a patch ( https://reviews.llvm.org/D146721) with a trivial Device Interface. We can extend it later on and add attribute interfaces if required. It has two functions from one of @domada 's patch. As it is, the only benefits the interface gives are i) Put all the device related functions into a single place ii) Hide how the values are get and set (uses the module attributes that you have decided to use) iii) Makes it easy to change if there is a different mechanism iv) Also allows for an alternative implementation for other modules where these values are available differently. Please let me know how this looks. Also, given that this is straightforward I would also like to check whether I am missing anything.

HI Kiran, no worries, thank you very much for spending the time you could on it! I personally think it's quite a good method of doing things as far as getting and setting things go. If I am correct in my understanding (which perhaps I am not) it would alter this current patch by moving the set/get methods into the interface rather than the OpenMPDialect, but the attribute definition inside of OpenMPOps.td would remain the same? And would the lowering of module attributes within the OpenMPDialect change at all? or would it very much just be the same case of checking if the attribute exists on the operation as far as everything else is concerned (my assumption is it would be the same with my current knowledge of interfaces, I just want to double check).

In D144896#4213015, @kiranchandramohan wrote:

In D144896#4212840, @agozillon wrote:

In D144896#4211723, @jdoerfert wrote:

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

Sorry for the delay here. I would like to just have a quick check whether having an interface is possible. If not, we can continue with this. I will come back to this tomorrow.

In D144896#4212840, @agozillon wrote:

In D144896#4211723, @jdoerfert wrote:

Sorry to ping

Pinging every week is not too much but considered OK.

FWIW, I think the patch is reasonable. I would remove the "rtl" part, and probably just call it "omp.flags" but otherwise it makes sense.
That said, @kiranchandramohan should give his opinion.

Thank you @jdoerfert I'll keep that in mind for the future!

Thank you, I'll wait on @kiranchandramohan's opinion before I update the name then, so as to get the next series of review comments done in a single update.

You can test the TODOs if you want by adding tests like the following.
https://github.com/llvm/llvm-project/blob/main/flang/test/Lower/OpenMP/Todo/reduction-allocatable.f90

The problem with doing this in two stages : i.e during lowering and a pass are that there are two representations for the same thing in the IR. One where the nested functions are not marked, and the other where they are not marked.

LGTM. Please wait a day before submission. Move the flang portion to a separate commit.

Thanks @MatsPetersson for working on this. I have started going through the patch and have some comments.

@jdoerfert Would using functions from compiler-rt cause issues with target offloading?
Context: We are thinking of using functions from compiler-rt for implementing the complex division operation. The existing implementations in Flang (an inline one) and the one on MLIR both have precision issues whereas the compiler-rt ones work fine.

@labrinea Is this patch the top-level patch for recursive function-specialization that benefits exchange2? Is there any setting required or would this perform specialization by default? Also, assuming this is with llvm-project/flang.

LG. Please wait for and OK from @jrtc27.

In D145808#4190001, @vzakhari wrote:

I wonder whether intermediate computations generated by ComplexToStandard conversion complex::DivOp honor the range of the element's data type. Maybe we can think of replacing the FIR operation with the one from the Complex dialect?

Fixes one of the issues in https://github.com/llvm/llvm-project/issues/61342.

LG.

@peixin Thanks for working on this. Could you check why the patch application is failing?

Adding @vdonaldson also to check whether this is the right way to do this and also for the interaction with the block construct.

Sorry, I haven't had a chance to look at this in detail. I would need to get up to speed with addrspace in LLVM before proceeding with this. I would recommend someone from your organization review this patch and then @jeanPerier sign this off, if it is OK.

LG.

In D144864#4175332, @agozillon wrote:

In D144864#4175257, @kiranchandramohan wrote:

LG. See one minor comment in the tests.

I would prefer having an Interface for Target Modules if that could be made to work. I guess this can be taken up separately after https://reviews.llvm.org/D144883.

Thank you Kiran, by interface for Target Modules do you mean a new omp.module or this type of external interface: https://mlir.llvm.org/docs/Interfaces/#external-models-for-attribute-operation-and-type-interfaces

And quite possibly, @skatrak is currently looking into the review comments in the referenced patch (as he found similar use for the patch), so we shall see where the results lead to. Never opposed to improving on the infrastructure and this is something we will iterate on as we progress with offloading! :)

LG. See one minor comment in the tests.

Thanks for the update and the replies. See comments inline.

Could you add a description in the summary or add a comment in the changed code on what this change does and how it fixes the issue?

I have some more comments.

Thanks for making the change.
I am still going through the patch, I have a few comments.

In D144013#4164369, @TIFitis wrote:

In D144013#4158654, @kiranchandramohan wrote:

In D144013#4158333, @TIFitis wrote:

In D144013#4156618, @kiranchandramohan wrote:

It will also be helpful if you can add TODOs for the types that are not handled.

Hi, can you please tell me which types you are referring to here?

All types that can be present in a map clause. Should be most of the following, please consult the restrictions for the clause.
char, char array
integer, real, complex and arrays of these with both fixed and variable length
pointer, allocatable, assumed shape arrays
derived type
derived type components, array-sections
polymorphic

Yes all these types should work for map operands. Is this patch good to go?

LGTM.

Change name of operation in assert to fix CI failure.

The other option is for Modules to implement an interface and then the Translation can detect this and forward it to the OpenMP Dialect translation in OpenMPToLLVMIRTranslation.cpp.

Thanks for the changes and the replies.

In D144013#4158333, @TIFitis wrote:

In D144013#4156618, @kiranchandramohan wrote:

It will also be helpful if you can add TODOs for the types that are not handled.

Hi, can you please tell me which types you are referring to here?

LGTM. Please wait for @awarzynski.

Thanks @luporl for having a look. There are a few fixes that are going in the privatisation area. I think the issue you are pointing out with your example will also be fixed by that. This fix is not causing the issue that you are seeing.

It will also be helpful if you can add TODOs for the types that are not handled.

Could you update the discourse thread (https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998) with the chosen approach and reasons before proceeding?

Could you update the discourse thread (https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998) with the chosen approach and reasons before proceeding?

Thanks. LGTM.

This will need a discussion with the FIR team.

ping @mnadeem. Do you need any inputs or helpf to complete this patch?

Thanks, that looks good. The runtime is in charge of mapping and copying the data. I guess it will transparently map the address from the host pointer to the device address space since it stores the mapping.

Note to self: Please add the relevant tests in flang/test/Fir/convert-to-llvm-openmp-and-fir.fir and mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir before submitting.