Skip to content

[flang][cuda] Adding atomicadd as a cudadevice intrinsic and converting it LLVM dialect #123840

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.

Sign up for GitHub

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

Merged
Renaud-K merged 3 commits into from
Jan 23, 2025
Merged

[flang][cuda] Adding atomicadd as a cudadevice intrinsic and converting it LLVM dialect #123840

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44a05d0
Adding atomicadd as a cudadevice intrinsic and converting it LLVM dia…
Renaud-K Jan 21, 2025
18120ce
Corrected includes added by clangd
Renaud-K Jan 21, 2025
e6b1268
Review feedback
Renaud-K Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions flang/include/flang/Optimizer/Builder/IntrinsicCall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ struct IntrinsicLibrary {
mlir::Value genAnint(mlir::Type, llvm::ArrayRef<mlir::Value>);
fir::ExtendedValue genAny(mlir::Type, llvm::ArrayRef<fir::ExtendedValue>);
mlir::Value genAtanpi(mlir::Type, llvm::ArrayRef<mlir::Value>);
mlir::Value genAtomAdd(mlir::Type, llvm::ArrayRef<mlir::Value>);
Copy link
Contributor

@clementval clementval Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to truncate Atomic to Atom?

Copy link
Contributor Author

@Renaud-K Renaud-K Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting. It will make each line become 3 lines. I found it a lot less readable.

Copy link
Contributor

@clementval clementval Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it fits here. Are you talking about about the implementation or in the table?

Copy link
Contributor

@jeanPerier jeanPerier Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As someone that spending a lot of time grepping, I would much rather having the full "atomic" spelled at the cost of extra lines in the table.

fir::ExtendedValue
genCommandArgumentCount(mlir::Type, llvm::ArrayRef<fir::ExtendedValue>);
mlir::Value genAsind(mlir::Type, llvm::ArrayRef<mlir::Value>);
Expand Down
26 changes: 25 additions & 1 deletion flang/lib/Optimizer/Builder/IntrinsicCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@
#include "flang/Runtime/iostat-consts.h"
#include "mlir/Dialect/Complex/IR/Complex.h"
#include "mlir/Dialect/LLVMIR/LLVMDialect.h"
#include "mlir/Dialect/LLVMIR/LLVMTypes.h"
#include "mlir/Dialect/Math/IR/Math.h"
#include "mlir/Dialect/Vector/IR/VectorOps.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/raw_ostream.h"
#include <cfenv> // temporary -- only used in genIeeeGetOrSetModesOrStatus
#include <mlir/IR/Value.h>
#include <optional>

#define DEBUG_TYPE "flang-lower-intrinsic"
Expand Down Expand Up @@ -147,6 +147,10 @@ static constexpr IntrinsicHandler handlers[]{
{"atan2pi", &I::genAtanpi},
{"atand", &I::genAtand},
{"atanpi", &I::genAtanpi},
{"atomicaddd", &I::genAtomAdd, {{{"addr", asAddr}, {"v", asValue}}}, false},
Copy link
Contributor

@jeanPerier jeanPerier Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, we should maybe move the CUF intrinsic module procedures into there own table (a bit like what is done for ppc intrinsics).

This would clarify what are regular Fortran intrinsics expected to be intercepted here, and what are CUF extensions (and if well done, could speed-up the lookup by only looking into the right table based on the intrinsic module).

You can leave that alone, I will try to split the table later (maybe ieee proc could also be taken out).

{"atomicaddf", &I::genAtomAdd, {{{"addr", asAddr}, {"v", asValue}}}, false},
{"atomicaddi", &I::genAtomAdd, {{{"addr", asAddr}, {"v", asValue}}}, false},
{"atomicaddl", &I::genAtomAdd, {{{"addr", asAddr}, {"v", asValue}}}, false},
{"bessel_jn",
&I::genBesselJn,
{{{"n1", asValue}, {"n2", asValue}, {"x", asValue}}},
Expand Down Expand Up @@ -2574,6 +2578,26 @@ mlir::Value IntrinsicLibrary::genAtanpi(mlir::Type resultType,
return builder.create<mlir::arith::MulFOp>(loc, atan, factor);
}

static mlir::Value genAtomBinOp(fir::FirOpBuilder &builder, mlir::Location &loc,
mlir::LLVM::AtomicBinOp binOp, mlir::Value arg0,
mlir::Value arg1) {
auto llvmPointerType = mlir::LLVM::LLVMPointerType::get(builder.getContext());
Copy link
Contributor

@jeanPerier jeanPerier Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, do you now if FIR alias analysis is clever enough to go through convert from this LLVM type and deduce what the llvm.atomicrmw is reading/writing not (or mainly, to deduce what it is not reading/writing to)?

It makes sense to me that we interoperate with LLVM pointer type to avoid redefining all the intrinsics at the FIR level, I just wonder if we need to improve AA here.

Copy link
Contributor Author

@Renaud-K Renaud-K Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always skip through fir.convert

        .Case<fir::ConvertOp>([&](auto op) {
          // Skip ConvertOp's and track further through the operand.
          v = op->getOperand(0);
          defOp = v.getDefiningOp();
        })
...

I was trying to make sense of the MemoryEffects of LLVM_AtomicRMWOp but couldn't quite make sense of it:

def LLVM_AtomicRMWOp : LLVM_MemAccessOpBase<"atomicrmw", [
      TypesMatchWith<"result #0 and operand #1 have the same type",
                     "val", "res", "$_self">]> {

We talked about these ops in the office hours today and while we need the hooks in place to create the appropriate instructions we may eventually replace them with higher level atomic operations. TDB.

Copy link
Contributor

@jeanPerier jeanPerier Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the effect description is not straightforward. If found this patch that explains some of it: feb7bea

From what I understand, it is giving atomic operation a global write effects because the synchronization aspect (e.g., could write some global lock).

arg0 = builder.createConvert(loc, llvmPointerType, arg0);
return builder.create<mlir::LLVM::AtomicRMWOp>(
loc, binOp, arg0, arg1, mlir::LLVM::AtomicOrdering::seq_cst);
}

mlir::Value IntrinsicLibrary::genAtomAdd(mlir::Type resultType,
llvm::ArrayRef<mlir::Value> args) {
assert(args.size() == 2);

mlir::LLVM::AtomicBinOp binOp =
mlir::isa<mlir::IntegerType>(args[1].getType())
? mlir::LLVM::AtomicBinOp::add
: mlir::LLVM::AtomicBinOp::fadd;
return genAtomBinOp(builder, loc, binOp, args[0], args[1]);
}

// ASSOCIATED
fir::ExtendedValue
IntrinsicLibrary::genAssociated(mlir::Type resultType,
Expand Down
26 changes: 26 additions & 0 deletions flang/module/cudadevice.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,31 @@ attributes(device) subroutine threadfence_system()
end function
end interface
public :: __fadd_ru

! Atomic Operations

interface atomicadd
attributes(device) pure integer function atomicaddi(address, val)
!dir$ ignore_tkr (rd) address, (d) val
Copy link
Contributor

@jeanPerier jeanPerier Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that one is allowed to call atomicadd with an address of any rank ("r"). What is the semantic of calling them with an array? Is it that the first element is modified, or that all elements are modified?

Copy link
Contributor Author

@Renaud-K Renaud-K Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brent mentioned 1 element arrays but could not be definitive. I can remove it and maybe we will learn something.

integer, intent(inout) :: address
integer, value :: val
end function
attributes(device) pure real function atomicaddf(address, val)
!dir$ ignore_tkr (rd) address, (d) val
real, intent(inout) :: address
real, value :: val
end function
attributes(device) pure real*8 function atomicaddd(address, val)
!dir$ ignore_tkr (rd) address, (d) val
real*8, intent(inout) :: address
real*8, value :: val
end function
attributes(device) pure integer(8) function atomicaddl(address, val)
!dir$ ignore_tkr (rd) address, (dk) val
integer(8), intent(inout) :: address
integer(8), value :: val
end function
end interface
public :: atomicadd

end module
13 changes: 13 additions & 0 deletions flang/test/Lower/CUDA/cuda-device-proc.cuf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
attributes(global) subroutine devsub()
implicit none
integer :: ret
real(4) :: af
real(8) :: ad
integer(4) :: ai
integer(8) :: al

call syncthreads()
call syncwarp(1)
Expand All @@ -14,6 +18,11 @@ attributes(global) subroutine devsub()
ret = syncthreads_and(1)
ret = syncthreads_count(1)
ret = syncthreads_or(1)

ai = atomicadd(ai, 1_4)
al = atomicadd(al, 1_8)
af = atomicadd(af, 1.0_4)
ad = atomicadd(ad, 1.0_8)
end

! CHECK-LABEL: func.func @_QPdevsub() attributes {cuf.proc_attr = #cuf.cuda_proc<global>}
Expand All @@ -25,6 +34,10 @@ end
! CHECK: %{{.*}} = fir.call @llvm.nvvm.barrier0.and(%c1_i32_0) fastmath<contract> : (i32) -> i32
! CHECK: %{{.*}} = fir.call @llvm.nvvm.barrier0.popc(%c1_i32_1) fastmath<contract> : (i32) -> i32
! CHECK: %{{.*}} = fir.call @llvm.nvvm.barrier0.or(%c1_i32_2) fastmath<contract> : (i32) -> i32
! CHECK: %{{.*}} = llvm.atomicrmw add %{{.*}}, %{{.*}} seq_cst : !llvm.ptr, i32
! CHECK: %{{.*}} = llvm.atomicrmw add %{{.*}}, %{{.*}} seq_cst : !llvm.ptr, i64
! CHECK: %{{.*}} = llvm.atomicrmw fadd %{{.*}}, %{{.*}} seq_cst : !llvm.ptr, f32
! CHECK: %{{.*}} = llvm.atomicrmw fadd %{{.*}}, %{{.*}} seq_cst : !llvm.ptr, f64

! CHECK: func.func private @llvm.nvvm.barrier0()
! CHECK: func.func private @__syncwarp(!fir.ref<i32> {cuf.data_attr = #cuf.cuda<device>}) attributes {cuf.proc_attr = #cuf.cuda_proc<device>, fir.bindc_name = "__syncwarp", fir.proc_attrs = #fir.proc_attrs<bind_c>}
Expand Down
9 changes: 9 additions & 0 deletions flang/test/Semantics/cuf-device-procedures01.cuf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,17 @@ end
! CHECK: threadfence_system (Subroutine): Use from threadfence_system in cudadevice

subroutine host()
real(4) :: af
real(8) :: ad
integer(4) :: ai
integer(8) :: al
call syncthreads()
ai = atomicadd(ai, 1_4)
al = atomicadd(al, 1_8)
af = atomicadd(af, 1.0_4)
ad = atomicadd(ad, 1.0_8)
end subroutine

! CHECK-LABEL: Subprogram scope: host
! CHECK: atomicadd, EXTERNAL: HostAssoc{{$}}
! CHECK: syncthreads, EXTERNAL: HostAssoc{{$}}
Loading