This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Added lowering support for atomic read and write constructs
ClosedPublic

Authored by NimishMishra on Mar 30 2022, 5:16 AM.

Details

Summary

This patch adds lowering support for atomic read and write constructs.

Co-authored-by: Kiran Chandramohan <kiran.chandramohan@arm.com>

Diff Detail

Event Timeline

NimishMishra created this revision.Mar 30 2022, 5:16 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
NimishMishra requested review of this revision.Mar 30 2022, 5:16 AM

@rriddle @ftynse Could you please look at the MLIR side of this patch? Concretely, without the changes done there, atomic read and write constructs were giving builtin.unrealized_conversion_cast for the variables used within them. An example I give below:

program sample
     use omp_lib

     integer::x,v
     !$omp atomic read
         x = v
 end program sample
llvm.func @_QQmain() {
    %0 = llvm.mlir.constant(1 : i64) : i64
    %1 = llvm.alloca %0 x i32 {bindc_name = "v", in_type = i32, operand_segment_sizes = dense<0> : vector<2xi32>, uniq_name = "_QFEv"} : (i64) -> !llvm.ptr<i32>
    %2 = builtin.unrealized_conversion_cast %1 : !llvm.ptr<i32> to !fir.ref<i32>
    %3 = llvm.mlir.constant(1 : i64) : i64
    %4 = llvm.alloca %3 x i32 {bindc_name = "x", in_type = i32, operand_segment_sizes = dense<0> : vector<2xi32>, uniq_name = "_QFEx"} : (i64) -> !llvm.ptr<i32>
    %5 = builtin.unrealized_conversion_cast %4 : !llvm.ptr<i32> to !fir.ref<i32>
    omp.atomic.read %5 = %2   : !fir.ref<i32>
    llvm.return
  }

We technically allowed the conversion for these operations (having no region attached to them).

shraiysh added inline comments.Mar 30 2022, 10:31 PM
flang/lib/Lower/OpenMP.cpp
552

Make this ClauseMemoryOrderKindAttr instead of StringAttr, and accept ClauseMemoryOrderKindAttr in the genOmpAtomic..Clauses(). Do not create a StringAttr internally, instead directly create a ClauseMemoryOrderKindAttr using the corresponding enum.

583

Thi check is not necessary. Just replace hint ? nullptr : hint with hint.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
49

Instead of individual structs, can we do something using templates like RegionlessOpConversion<T> and use it like RegionOpConversion is used. Avoids code duplication. Feel free to choose a better name, but I think it would be reusable if we could template this..

The MLIR side LGTM

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
54
56

nit: Drop the newline here (same for the other pattern)

68
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3376–3382
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
102–103

Can you add the new legalisation conditions here as well? It might be better for code reuse and the keep things in sync to create a new function in this file for the legalisation conditions and call that from CodeGen.cpp in Flang.

Add a test with the relevant op from the memref dialect if it is supported i.e an OpenMP to LLVM conversion test with an omp.atomic.read op having the memref type as its argument.

NimishMishra marked 6 inline comments as done.

Addressed comments

NimishMishra added inline comments.Apr 1 2022, 10:20 PM
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
54

I do not understand why but this change isn't working. A lot of errors come in, but they stem from OpAdaptor not declared. I even tried with typename omp::AtomicReadOp::OpAdaptor adaptor and that didn't work.

102–103

I don't think the memref dialect is supported as yet. If I give a memref argument to omp.atomic.read, it just says that's an incompatible argument.

peixin added inline comments.Apr 2 2022, 2:35 AM
flang/test/Lower/OpenMP/atomic02.f90
2

Please use bbc here. The same reason in https://reviews.llvm.org/D122595 (Check the most recent comment).

Shifted tests to use bbc instead of flang-new

NimishMishra marked an inline comment as done.Apr 3 2022, 9:51 PM

LGTM

flang/lib/Lower/OpenMP.cpp
459

We won't need foundMemoryOrderClause as an argument if we would take ClauseMemoryOrderKindAttr instead of ClauseMemoryOrderKind in the function arguments. ClauseMemoryOrderKindAttr can stay nullptr when the memory order clause is not found.

546

Change this to ClauseMemoryOrderKindAttr.

579–581

Remove this condition once the other changes for memory_order are done.

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
22–24

Is this required? This looks like a function that is only used in this cpp file only.

NimishMishra marked 3 inline comments as done.

Addressed comments.

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
22–24

Yes. This function is also called from CodeGen.cpp

peixin added inline comments.Apr 4 2022, 10:39 AM
flang/lib/Lower/OpenMP.cpp
468

Why nullptr for hint(0)?

flang/test/Lower/OpenMP/atomic02.f90
17

nit: c7_i32 -> [[CONST_7]]?

NimishMishra added inline comments.Apr 5 2022, 6:10 AM
flang/lib/Lower/OpenMP.cpp
468

The FIR generation is not distinguishing between hint(0) and nullptr. So when I pass a hint(0) and emit FIR, it ends up as hint() and causes problems during further lowering. That's why I have made this change to make no distinction between a hint(0) and an absence of hint clause.

clementval added inline comments.Apr 5 2022, 6:50 AM
flang/lib/Optimizer/Dialect/FIRType.cpp
15

I find it weird to have OpenMP things coming into FIR types.

clementval added inline comments.
flang/lib/Optimizer/Dialect/FIRType.cpp
15

To me is kind of go against the concept of distinct dialects. What do you think @schweitz?

schweitz added inline comments.Apr 5 2022, 8:30 AM
flang/lib/Optimizer/Dialect/FIRType.cpp
15

I agree. The FIR dialect should not depend on the OpenMP dialect any more than Fortran language depends on the OpenMP definition.

peixin added inline comments.Apr 5 2022, 8:37 AM
flang/lib/Lower/OpenMP.cpp
468

It seems that this is one bug here. Will post it on slack. It is also related to critical construct.

schweitz added inline comments.Apr 5 2022, 8:40 AM
flang/lib/Lower/OpenMP.cpp
468

Do you want to use a function other than ToInt64 to distinguish between a 0 and the absence of an argument? Or is the problem that an absent argument is always promoted to 0 before it gets here?

flang/lib/Optimizer/Dialect/FIRType.cpp
918

It probably makes sense to move this pointer modeling code into the "common code for tools" area and not add a dependency between FIR dialect and OpenMP dialect here.

clementval requested changes to this revision.Apr 5 2022, 9:46 AM

Remove dependency between dialects.

This revision now requires changes to proceed.Apr 5 2022, 9:46 AM
rriddle added inline comments.Apr 5 2022, 9:48 AM
mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
11

Is this include necessary?

22

Please use proper sentences for documentation.

23

Can you nest this in a different namespace or rename it?

This is a very generically named function to live in mlir.

shraiysh added inline comments.Apr 6 2022, 12:02 AM
flang/lib/Lower/OpenMP.cpp
468

Do you want to use a function other than ToInt64 to distinguish between a 0 and the absence of an argument?

I don't think that is required but fir-dev treats these as separate cases, which is the cause of confusion. I will add it here too at the moment for consistency.

NimishMishra added inline comments.Apr 6 2022, 12:47 AM
flang/lib/Optimizer/Dialect/FIRType.cpp
918

I understand that we need to decouple things here. However, can you explain a little where exactly this "common code for tools" are is? I don't suppose it would be flang/lib/Common, would it be?

flang/lib/Optimizer/Dialect/FIRType.cpp
918

It could be include/flang/Optimizer/Support/InitFIR.h or the related cpp file if by tools @schweitz meant the driver tools (bbc,tco,flang-new etc).

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
23

The GPU dialect uses a name like configureGpuToNVVMConversionLegality(ConversionTarget &target). Can we do something similar?

schweitz added inline comments.Apr 6 2022, 8:43 AM
flang/lib/Optimizer/Dialect/FIRType.cpp
918

I was thinking of include/flang/Tools or possibly a new tools/? subdirectory.

NimishMishra marked 3 inline comments as done.
NimishMishra added inline comments.
flang/lib/Lower/OpenMP.cpp
468

This problem is resolved through the currently under-review patch https://reviews.llvm.org/D123186

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
11

I think so. We wanted to have ConversionTarget visible here in order to define the function we did below.

flang/include/flang/Tools/PointerModels.h
2

Please add the licensing and other headers as well as the include guards.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3372

Please update this comment with info about operations without regions as well. Or may be we can just say legalize OpenMP operations here and move the details to comments in the called function.

rriddle added inline comments.Apr 11 2022, 10:27 AM
mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
11

Please prefer using forward declarations instead of includes in header files. It doesn't look like the full definition of ConversionTarget is needed here.

NimishMishra marked 3 inline comments as done.

Addressed comments.

flang/lib/Optimizer/Dialect/FIRType.cpp
15

@schweitz @clementval Is this location OK?

918

Is the change OK? Or do you want to move the attachInterface also to another place?

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
102–103

The CI fails. Can this be at high priority? The threadprivate directive lowering needs some code of this PR. The default clause lowering needs some code from the threadprivate directive lowering.

NimishMishra added inline comments.Apr 19 2022, 10:14 PM
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
102–103

I am trying to attach the following test case in flang/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir

func @atomic_read(%v: memref<i32>, %x: memref<i32>){
       omp.atomic.read %v = %x : memref <i32>
       return
}

The error I get is that operation operand must be OpenMP-compatible variable type, but got !llvm.struct<(ptr<i32>, ptr<i32>, i64)>. If we further investigate the operands to the atomic operation, they lower to unrealised conversion cast operations.

Fixed CI failures/

LGTM. Please wait for approval from @clementval/@schweitz for the location of attaching the OpenMP interface and any outstanding comments from the other reviewers.

flang/include/flang/Tools/PointerModels.h
1

Nit: I think this can just be PointerModels.h or Tools/PointerModels.h.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
102–103

Ahh OK. I missed that point. We will need to do something similar to what OpenACC does in handling these memref types so that the LLVM conversion works fine for them. But it is definitely not in the scope of this patch.
https://github.com/llvm/llvm-project/blob/d7565de6cc6ba4de8a5d73282281ff95d7a0ad46/mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp#L100

LGTM. Please wait for approval from @clementval/@schweitz for the location of attaching the OpenMP interface and any outstanding comments from the other reviewers.

I'm good with the location of PointerModels.h. Thanks for breaking the dependence.

LGTM. Please wait for approval from @clementval/@schweitz for the location of attaching the OpenMP interface and any outstanding comments from the other reviewers.

Ok for me too.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 20 2022, 11:50 PM
This revision was automatically updated to reflect the committed changes.