-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[flang][flang-driver][mlir][OpenMP] atomic control support #143441
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -53,6 +53,11 @@ class TargetOptions { | |||||||||||||
|
||||||||||||||
/// Print verbose assembly | ||||||||||||||
bool asmVerbose = false; | ||||||||||||||
|
||||||||||||||
/// Atomic control options for AMD gpu | ||||||||||||||
bool ignoreDenormalMode = false; | ||||||||||||||
bool remoteMemory = false; | ||||||||||||||
bool fineGrainedMemory = false; | ||||||||||||||
Comment on lines
+58
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think it makes sense to include "atomic" within variables names and also associated getters/setters and attributes, to avoid confusion as to what these flags apply to.
Suggested change
|
||||||||||||||
}; | ||||||||||||||
|
||||||||||||||
} // end namespace Fortran::frontend | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -58,6 +58,14 @@ void setTargetCPU(mlir::ModuleOp mod, llvm::StringRef cpu); | |||||
/// Get the target CPU string from the Module or return a null reference. | ||||||
llvm::StringRef getTargetCPU(mlir::ModuleOp mod); | ||||||
|
||||||
// Setters and getters for atomic control options. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: All other getters/setters here are documented individually using doxygen-formatted comments. I'd suggest doing the same, even if a chunk of the description is going to repeat across multiple functions. |
||||||
void setIgnoreDenormalMode(mlir::ModuleOp mod); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setters should allow us to specify a value. Their implementation, when set to
Suggested change
|
||||||
bool getIgnoreDenormalMode(mlir::ModuleOp mod); | ||||||
void setFineGrainedMemory(mlir::ModuleOp mod); | ||||||
bool getFineGrainedMemory(mlir::ModuleOp mod); | ||||||
void setRemoteMemory(mlir::ModuleOp mod); | ||||||
bool getRemoteMemory(mlir::ModuleOp mod); | ||||||
|
||||||
/// Set the tune CPU for the module. `cpu` must not be deallocated while | ||||||
/// module `mod` is still live. | ||||||
void setTuneCPU(mlir::ModuleOp mod, llvm::StringRef cpu); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -6654,6 +6654,12 @@ Fortran::lower::LoweringBridge::LoweringBridge( | |||||||
fir::setKindMapping(*module, kindMap); | ||||||||
fir::setTargetCPU(*module, targetMachine.getTargetCPU()); | ||||||||
fir::setTuneCPU(*module, targetOpts.cpuToTuneFor); | ||||||||
if (targetOpts.ignoreDenormalMode) | ||||||||
fir::setIgnoreDenormalMode(*module); | ||||||||
Comment on lines
+6657
to
+6658
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
if (targetOpts.fineGrainedMemory) | ||||||||
fir::setFineGrainedMemory(*module); | ||||||||
if (targetOpts.remoteMemory) | ||||||||
fir::setRemoteMemory(*module); | ||||||||
fir::setTargetFeatures(*module, targetMachine.getTargetFeatureString()); | ||||||||
fir::support::setMLIRDataLayout(*module, targetMachine.createDataLayout()); | ||||||||
fir::setIdent(*module, Fortran::common::getFlangFullVersion()); | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -88,6 +88,36 @@ void fir::setTuneCPU(mlir::ModuleOp mod, llvm::StringRef cpu) { | |||||
mod->setAttr(tuneCpuName, mlir::StringAttr::get(ctx, cpu)); | ||||||
} | ||||||
|
||||||
static constexpr const char *ignoreDenormalModeName = | ||||||
"fir.ignore.denormal.mode"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Generally, dots are used to separate "scopes" in some hierarchy. I'd suggest following the
Suggested change
|
||||||
void fir::setIgnoreDenormalMode(mlir::ModuleOp mod) { | ||||||
auto *ctx = mod.getContext(); | ||||||
mod->setAttr(ignoreDenormalModeName, mlir::UnitAttr::get(ctx)); | ||||||
} | ||||||
|
||||||
bool fir::getIgnoreDenormalMode(mlir::ModuleOp mod) { | ||||||
return mod->hasAttrOfType<mlir::UnitAttr>(ignoreDenormalModeName); | ||||||
} | ||||||
|
||||||
static constexpr const char *fineGrainedMemoryName = "fir.fine.grained.memory"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
void fir::setFineGrainedMemory(mlir::ModuleOp mod) { | ||||||
auto *ctx = mod.getContext(); | ||||||
mod->setAttr(fineGrainedMemoryName, mlir::UnitAttr::get(ctx)); | ||||||
} | ||||||
|
||||||
bool fir::getFineGrainedMemory(mlir::ModuleOp mod) { | ||||||
return mod->hasAttrOfType<mlir::UnitAttr>(fineGrainedMemoryName); | ||||||
} | ||||||
static constexpr const char *remoteMemoryName = "fir.remote.memory"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
void fir::setRemoteMemory(mlir::ModuleOp mod) { | ||||||
auto *ctx = mod.getContext(); | ||||||
mod->setAttr(remoteMemoryName, mlir::UnitAttr::get(ctx)); | ||||||
} | ||||||
|
||||||
bool fir::getRemoteMemory(mlir::ModuleOp mod) { | ||||||
return mod->hasAttrOfType<mlir::UnitAttr>(remoteMemoryName); | ||||||
} | ||||||
|
||||||
llvm::StringRef fir::getTuneCPU(mlir::ModuleOp mod) { | ||||||
if (auto attr = mod->getAttrOfType<mlir::StringAttr>(tuneCpuName)) | ||||||
return attr.getValue(); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
! RUN: %flang_fc1 -emit-hlfir -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -munsafe-fp-atomics %s -o - | FileCheck -check-prefix=UNSAFE-FP-ATOMICS %s | ||
! RUN: %flang_fc1 -emit-hlfir -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-ignore-denormal-mode %s -o - | FileCheck -check-prefix=IGNORE-DENORMAL %s | ||
! RUN: %flang_fc1 -emit-hlfir -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-fine-grained-memory %s -o - | FileCheck -check-prefix=FINE-GRAINED-MEMORY %s | ||
! RUN: %flang_fc1 -emit-hlfir -triple amdgcn-amd-amdhsa -fopenmp -fopenmp-is-device -fatomic-remote-memory %s -o - | FileCheck -check-prefix=REMOTE-MEMORY %s | ||
program test | ||
implicit none | ||
integer :: A, B, threads | ||
threads = 128 | ||
A = 0 | ||
B = 0 | ||
!UNSAFE-FP-ATOMICS: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!UNSAFE-FP-ATOMICS: } {atomic_control = #omp.atomic_control<ignore_denormal_mode = true>} | ||
!IGNORE-DENORMAL: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!IGNORE-DENORMAL: } {atomic_control = #omp.atomic_control<ignore_denormal_mode = true>} | ||
!FINE-GRAINED-MEMORY: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!FINE-GRAINED-MEMORY: } {atomic_control = #omp.atomic_control<fine_grained_memory = true>} | ||
!REMOTE-MEMORY: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!REMOTE-MEMORY: } {atomic_control = #omp.atomic_control<remote_memory = true>} | ||
!$omp target parallel num_threads(threads) | ||
!$omp atomic | ||
A = A + 1 | ||
!$omp end target parallel | ||
!UNSAFE-FP-ATOMICS: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!UNSAFE-FP-ATOMICS: } {atomic_control = #omp.atomic_control<ignore_denormal_mode = true>} | ||
!IGNORE-DENORMAL: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!IGNORE-DENORMAL: } {atomic_control = #omp.atomic_control<ignore_denormal_mode = true>} | ||
!FINE-GRAINED-MEMORY: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!FINE-GRAINED-MEMORY: } {atomic_control = #omp.atomic_control<fine_grained_memory = true>} | ||
!REMOTE-MEMORY: omp.atomic.update %{{.*}} : !fir.ref<i32> { | ||
!REMOTE-MEMORY: } {atomic_control = #omp.atomic_control<remote_memory = true>} | ||
!$omp target parallel num_threads(threads) | ||
!$omp atomic capture | ||
A = A + B | ||
B = A | ||
!$omp end atomic | ||
!$omp end target parallel | ||
end program test |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,21 @@ def FlagsAttr : OpenMP_Attr<"Flags", "flags"> { | |
let assemblyFormat = "`<` struct(params) `>`"; | ||
} | ||
|
||
//===----------------------------------------------------------------------===// | ||
// AtomicControlAttr | ||
//===----------------------------------------------------------------------===// | ||
|
||
// Atomic control attributes hold information about architectural | ||
// characteristics which are required for lowering atomic operations. | ||
def AtomicControlAttr : OpenMP_Attr<"AtomicControl", "atomic_control"> { | ||
let parameters = | ||
(ins DefaultValuedParameter<"bool", "false">:$ignore_denormal_mode, | ||
DefaultValuedParameter<"bool", "false">:$fine_grained_memory, | ||
DefaultValuedParameter<"bool", "false">:$remote_memory); | ||
|
||
let assemblyFormat = "`<` struct(params) `>`"; | ||
} | ||
|
||
Comment on lines
+57
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This definition should go before |
||
//===----------------------------------------------------------------------===// | ||
// TaskDependArrayAttr | ||
//===----------------------------------------------------------------------===// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1543,9 +1543,11 @@ def AtomicUpdateOp : OpenMP_Op<"atomic.update", traits = [ | |
operations. | ||
}] # clausesDescription; | ||
|
||
let arguments = !con((ins Arg<OpenMP_PointerLikeType, | ||
"Address of variable to be updated", | ||
[MemRead, MemWrite]>:$x), clausesArgs); | ||
let arguments = !con( | ||
(ins Arg<OpenMP_PointerLikeType, | ||
"Address of variable to be updated", [MemRead, MemWrite]>:$x, | ||
AtomicControlAttr:$atomic_control), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have you made this attribute required? There's a clear default for it (all flags set to false) and, even in the unit tests you had to update because of this choice, the attribute is specified but empty. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this new attribute only actually apply to |
||
clausesArgs); | ||
|
||
// Override region definition. | ||
let regions = (region SizedRegion<1>:$region); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.