[Flang][OpenMP] Fix to resolve the crash with SIMD aligned clause. #150612
Conversation
kaviya2510
requested review from
tblah,
kiranchandramohan,
NimishMishra,
kiranktp and
Thirumalai-Shaktivel
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-llvm Author: Kaviya Rajendiran (kaviya2510) ChangesIssue: Fix: So instead of emitting Full diff: https://github.com/llvm/llvm-project/pull/150612.diff 6 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 594f95ecdda63..8f541f9bb4e03 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -647,10 +647,8 @@ addAlignedClause(lower::AbstractConverter &converter,
// The default alignment for some targets is equal to 0.
// Do not generate alignment assumption if alignment is less than or equal to
- // 0.
- if (alignment > 0) {
- // alignment value must be power of 2
- assert((alignment & (alignment - 1)) == 0 && "alignment is not power of 2");
+ // 0 or not a power of two
+ if (alignment > 0 && ((alignment & (alignment - 1)) == 0)) {
auto &objects = std::get<omp::ObjectList>(clause.t);
if (!objects.empty())
genObjectList(objects, converter, alignedVars);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index d214d222e7c90..c19abea5becf8 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3244,9 +3244,14 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Aligned &x) {
x.v, llvm::omp::OMPC_aligned, GetContext().clauseSource, context_)) {
auto &modifiers{OmpGetModifiers(x.v)};
if (auto *align{OmpGetUniqueModifier<parser::OmpAlignment>(modifiers)}) {
- if (const auto &v{GetIntValue(align->v)}; !v || *v <= 0) {
+ const auto &v{GetIntValue(align->v)};
+ if (!v || *v <= 0) {
context_.Say(OmpGetModifierSource(modifiers, align),
"The alignment value should be a constant positive integer"_err_en_US);
+ } else if (((*v) & (*v - 1)) != 0) {
+ context_.Warn(common::UsageWarning::OpenMPUsage,
+ OmpGetModifierSource(modifiers, align),
+ "Alignment is not a power of 2, Aligned clause will be ignored"_warn_en_US);
}
}
}
diff --git a/flang/test/Lower/OpenMP/simd.f90 b/flang/test/Lower/OpenMP/simd.f90
index d815474b84b31..7655c786573e3 100644
--- a/flang/test/Lower/OpenMP/simd.f90
+++ b/flang/test/Lower/OpenMP/simd.f90
@@ -226,6 +226,23 @@ subroutine simdloop_aligned_allocatable()
end do
end subroutine
+subroutine aligned_non_power_of_two()
+ integer :: i
+ integer, allocatable :: A(:)
+ allocate(A(10))
+!CHECK: %[[A_PTR:.*]] = fir.alloca !fir.box<!fir.heap<!fir.array<?xi32>>> {bindc_name = "a",
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"}
+!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %[[A_PTR]] {fortran_attrs = #fir.var_attrs<allocatable>,
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"} :
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
+!CHECK: omp.simd private
+ !$OMP SIMD ALIGNED(A:257)
+ do i = 1, 10
+ A(i) = i
+ end do
+end subroutine
+
!CHECK-LABEL: func @_QPsimd_with_nontemporal_clause
subroutine simd_with_nontemporal_clause(n)
!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFsimd_with_nontemporal_clauseEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
diff --git a/flang/test/Semantics/OpenMP/simd-aligned.f90 b/flang/test/Semantics/OpenMP/simd-aligned.f90
index 0a9f95833e22e..4c410a7c4631b 100644
--- a/flang/test/Semantics/OpenMP/simd-aligned.f90
+++ b/flang/test/Semantics/OpenMP/simd-aligned.f90
@@ -60,9 +60,16 @@ program omp_simd
!$omp end simd
!ERROR: 'd' in ALIGNED clause must be of type C_PTR, POINTER or ALLOCATABLE
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
!$omp simd aligned(d:100)
do i = 1, 100
d(i) = i
end do
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
+ !$omp simd aligned(b:65)
+ do i = 1, 100
+ b(i) = i
+ end do
+
end program omp_simd
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 9f18199c75b4b..48010c7ec1a14 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2893,6 +2893,11 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
alignment = builder.getInt64(intAttr.getInt());
assert(ty->isPointerTy() && "Invalid type for aligned variable");
assert(alignment && "Invalid alignment value");
+ // Check if the alignment value is not a power of 2. If so, skip emitting
+ // alignment.
+ if (!intAttr.getValue().isPowerOf2())
+ continue;
+
auto curInsert = builder.saveIP();
builder.SetInsertPoint(sourceBlock);
llvmVal = builder.CreateLoad(ty, llvmVal);
diff --git a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
index 234604e4b664a..fd19ca15f5df0 100644
--- a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
@@ -58,3 +58,21 @@ llvm.func @_QPsimd_aligned_allocatable() {
}
llvm.return
}
+
+//CHECK-LABEL: define void @_QPsimd_aligned_non_power_of_two() {
+//CHECK: %[[A_ADDR:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, i64 1, align 8
+//CHECK-NOT: call void @llvm.assume(i1 true) [ "align"(ptr %{{.*}}, i64 256) ]
+llvm.func @_QPsimd_aligned_non_power_of_two() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)> {bindc_name = "a"} : (i64) -> !llvm.ptr
+ %2 = llvm.mlir.constant(1 : i32) : i32
+ %3 = llvm.mlir.constant(10 : i32) : i32
+ %4 = llvm.mlir.constant(1 : i32) : i32
+ omp.simd aligned(%1 : !llvm.ptr -> 257 : i64) {
+ omp.loop_nest (%arg0) : i32 = (%2) to (%3) inclusive step (%4) {
+ omp.yield
+ }
+ }
+ llvm.return
+}
+
|
|
@llvm/pr-subscribers-flang-openmp Author: Kaviya Rajendiran (kaviya2510) ChangesIssue: Fix: So instead of emitting Full diff: https://github.com/llvm/llvm-project/pull/150612.diff 6 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 594f95ecdda63..8f541f9bb4e03 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -647,10 +647,8 @@ addAlignedClause(lower::AbstractConverter &converter,
// The default alignment for some targets is equal to 0.
// Do not generate alignment assumption if alignment is less than or equal to
- // 0.
- if (alignment > 0) {
- // alignment value must be power of 2
- assert((alignment & (alignment - 1)) == 0 && "alignment is not power of 2");
+ // 0 or not a power of two
+ if (alignment > 0 && ((alignment & (alignment - 1)) == 0)) {
auto &objects = std::get<omp::ObjectList>(clause.t);
if (!objects.empty())
genObjectList(objects, converter, alignedVars);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index d214d222e7c90..c19abea5becf8 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3244,9 +3244,14 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Aligned &x) {
x.v, llvm::omp::OMPC_aligned, GetContext().clauseSource, context_)) {
auto &modifiers{OmpGetModifiers(x.v)};
if (auto *align{OmpGetUniqueModifier<parser::OmpAlignment>(modifiers)}) {
- if (const auto &v{GetIntValue(align->v)}; !v || *v <= 0) {
+ const auto &v{GetIntValue(align->v)};
+ if (!v || *v <= 0) {
context_.Say(OmpGetModifierSource(modifiers, align),
"The alignment value should be a constant positive integer"_err_en_US);
+ } else if (((*v) & (*v - 1)) != 0) {
+ context_.Warn(common::UsageWarning::OpenMPUsage,
+ OmpGetModifierSource(modifiers, align),
+ "Alignment is not a power of 2, Aligned clause will be ignored"_warn_en_US);
}
}
}
diff --git a/flang/test/Lower/OpenMP/simd.f90 b/flang/test/Lower/OpenMP/simd.f90
index d815474b84b31..7655c786573e3 100644
--- a/flang/test/Lower/OpenMP/simd.f90
+++ b/flang/test/Lower/OpenMP/simd.f90
@@ -226,6 +226,23 @@ subroutine simdloop_aligned_allocatable()
end do
end subroutine
+subroutine aligned_non_power_of_two()
+ integer :: i
+ integer, allocatable :: A(:)
+ allocate(A(10))
+!CHECK: %[[A_PTR:.*]] = fir.alloca !fir.box<!fir.heap<!fir.array<?xi32>>> {bindc_name = "a",
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"}
+!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %[[A_PTR]] {fortran_attrs = #fir.var_attrs<allocatable>,
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"} :
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
+!CHECK: omp.simd private
+ !$OMP SIMD ALIGNED(A:257)
+ do i = 1, 10
+ A(i) = i
+ end do
+end subroutine
+
!CHECK-LABEL: func @_QPsimd_with_nontemporal_clause
subroutine simd_with_nontemporal_clause(n)
!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFsimd_with_nontemporal_clauseEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
diff --git a/flang/test/Semantics/OpenMP/simd-aligned.f90 b/flang/test/Semantics/OpenMP/simd-aligned.f90
index 0a9f95833e22e..4c410a7c4631b 100644
--- a/flang/test/Semantics/OpenMP/simd-aligned.f90
+++ b/flang/test/Semantics/OpenMP/simd-aligned.f90
@@ -60,9 +60,16 @@ program omp_simd
!$omp end simd
!ERROR: 'd' in ALIGNED clause must be of type C_PTR, POINTER or ALLOCATABLE
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
!$omp simd aligned(d:100)
do i = 1, 100
d(i) = i
end do
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
+ !$omp simd aligned(b:65)
+ do i = 1, 100
+ b(i) = i
+ end do
+
end program omp_simd
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 9f18199c75b4b..48010c7ec1a14 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2893,6 +2893,11 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
alignment = builder.getInt64(intAttr.getInt());
assert(ty->isPointerTy() && "Invalid type for aligned variable");
assert(alignment && "Invalid alignment value");
+ // Check if the alignment value is not a power of 2. If so, skip emitting
+ // alignment.
+ if (!intAttr.getValue().isPowerOf2())
+ continue;
+
auto curInsert = builder.saveIP();
builder.SetInsertPoint(sourceBlock);
llvmVal = builder.CreateLoad(ty, llvmVal);
diff --git a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
index 234604e4b664a..fd19ca15f5df0 100644
--- a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
@@ -58,3 +58,21 @@ llvm.func @_QPsimd_aligned_allocatable() {
}
llvm.return
}
+
+//CHECK-LABEL: define void @_QPsimd_aligned_non_power_of_two() {
+//CHECK: %[[A_ADDR:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, i64 1, align 8
+//CHECK-NOT: call void @llvm.assume(i1 true) [ "align"(ptr %{{.*}}, i64 256) ]
+llvm.func @_QPsimd_aligned_non_power_of_two() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)> {bindc_name = "a"} : (i64) -> !llvm.ptr
+ %2 = llvm.mlir.constant(1 : i32) : i32
+ %3 = llvm.mlir.constant(10 : i32) : i32
+ %4 = llvm.mlir.constant(1 : i32) : i32
+ omp.simd aligned(%1 : !llvm.ptr -> 257 : i64) {
+ omp.loop_nest (%arg0) : i32 = (%2) to (%3) inclusive step (%4) {
+ omp.yield
+ }
+ }
+ llvm.return
+}
+
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Kaviya Rajendiran (kaviya2510) ChangesIssue: Fix: So instead of emitting Full diff: https://github.com/llvm/llvm-project/pull/150612.diff 6 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 594f95ecdda63..8f541f9bb4e03 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -647,10 +647,8 @@ addAlignedClause(lower::AbstractConverter &converter,
// The default alignment for some targets is equal to 0.
// Do not generate alignment assumption if alignment is less than or equal to
- // 0.
- if (alignment > 0) {
- // alignment value must be power of 2
- assert((alignment & (alignment - 1)) == 0 && "alignment is not power of 2");
+ // 0 or not a power of two
+ if (alignment > 0 && ((alignment & (alignment - 1)) == 0)) {
auto &objects = std::get<omp::ObjectList>(clause.t);
if (!objects.empty())
genObjectList(objects, converter, alignedVars);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index d214d222e7c90..c19abea5becf8 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3244,9 +3244,14 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Aligned &x) {
x.v, llvm::omp::OMPC_aligned, GetContext().clauseSource, context_)) {
auto &modifiers{OmpGetModifiers(x.v)};
if (auto *align{OmpGetUniqueModifier<parser::OmpAlignment>(modifiers)}) {
- if (const auto &v{GetIntValue(align->v)}; !v || *v <= 0) {
+ const auto &v{GetIntValue(align->v)};
+ if (!v || *v <= 0) {
context_.Say(OmpGetModifierSource(modifiers, align),
"The alignment value should be a constant positive integer"_err_en_US);
+ } else if (((*v) & (*v - 1)) != 0) {
+ context_.Warn(common::UsageWarning::OpenMPUsage,
+ OmpGetModifierSource(modifiers, align),
+ "Alignment is not a power of 2, Aligned clause will be ignored"_warn_en_US);
}
}
}
diff --git a/flang/test/Lower/OpenMP/simd.f90 b/flang/test/Lower/OpenMP/simd.f90
index d815474b84b31..7655c786573e3 100644
--- a/flang/test/Lower/OpenMP/simd.f90
+++ b/flang/test/Lower/OpenMP/simd.f90
@@ -226,6 +226,23 @@ subroutine simdloop_aligned_allocatable()
end do
end subroutine
+subroutine aligned_non_power_of_two()
+ integer :: i
+ integer, allocatable :: A(:)
+ allocate(A(10))
+!CHECK: %[[A_PTR:.*]] = fir.alloca !fir.box<!fir.heap<!fir.array<?xi32>>> {bindc_name = "a",
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"}
+!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %[[A_PTR]] {fortran_attrs = #fir.var_attrs<allocatable>,
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"} :
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
+!CHECK: omp.simd private
+ !$OMP SIMD ALIGNED(A:257)
+ do i = 1, 10
+ A(i) = i
+ end do
+end subroutine
+
!CHECK-LABEL: func @_QPsimd_with_nontemporal_clause
subroutine simd_with_nontemporal_clause(n)
!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFsimd_with_nontemporal_clauseEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
diff --git a/flang/test/Semantics/OpenMP/simd-aligned.f90 b/flang/test/Semantics/OpenMP/simd-aligned.f90
index 0a9f95833e22e..4c410a7c4631b 100644
--- a/flang/test/Semantics/OpenMP/simd-aligned.f90
+++ b/flang/test/Semantics/OpenMP/simd-aligned.f90
@@ -60,9 +60,16 @@ program omp_simd
!$omp end simd
!ERROR: 'd' in ALIGNED clause must be of type C_PTR, POINTER or ALLOCATABLE
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
!$omp simd aligned(d:100)
do i = 1, 100
d(i) = i
end do
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
+ !$omp simd aligned(b:65)
+ do i = 1, 100
+ b(i) = i
+ end do
+
end program omp_simd
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 9f18199c75b4b..48010c7ec1a14 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2893,6 +2893,11 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
alignment = builder.getInt64(intAttr.getInt());
assert(ty->isPointerTy() && "Invalid type for aligned variable");
assert(alignment && "Invalid alignment value");
+ // Check if the alignment value is not a power of 2. If so, skip emitting
+ // alignment.
+ if (!intAttr.getValue().isPowerOf2())
+ continue;
+
auto curInsert = builder.saveIP();
builder.SetInsertPoint(sourceBlock);
llvmVal = builder.CreateLoad(ty, llvmVal);
diff --git a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
index 234604e4b664a..fd19ca15f5df0 100644
--- a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
@@ -58,3 +58,21 @@ llvm.func @_QPsimd_aligned_allocatable() {
}
llvm.return
}
+
+//CHECK-LABEL: define void @_QPsimd_aligned_non_power_of_two() {
+//CHECK: %[[A_ADDR:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, i64 1, align 8
+//CHECK-NOT: call void @llvm.assume(i1 true) [ "align"(ptr %{{.*}}, i64 256) ]
+llvm.func @_QPsimd_aligned_non_power_of_two() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)> {bindc_name = "a"} : (i64) -> !llvm.ptr
+ %2 = llvm.mlir.constant(1 : i32) : i32
+ %3 = llvm.mlir.constant(10 : i32) : i32
+ %4 = llvm.mlir.constant(1 : i32) : i32
+ omp.simd aligned(%1 : !llvm.ptr -> 257 : i64) {
+ omp.loop_nest (%arg0) : i32 = (%2) to (%3) inclusive step (%4) {
+ omp.yield
+ }
+ }
+ llvm.return
+}
+
|
|
@llvm/pr-subscribers-mlir-openmp Author: Kaviya Rajendiran (kaviya2510) ChangesIssue: Fix: So instead of emitting Full diff: https://github.com/llvm/llvm-project/pull/150612.diff 6 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 594f95ecdda63..8f541f9bb4e03 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -647,10 +647,8 @@ addAlignedClause(lower::AbstractConverter &converter,
// The default alignment for some targets is equal to 0.
// Do not generate alignment assumption if alignment is less than or equal to
- // 0.
- if (alignment > 0) {
- // alignment value must be power of 2
- assert((alignment & (alignment - 1)) == 0 && "alignment is not power of 2");
+ // 0 or not a power of two
+ if (alignment > 0 && ((alignment & (alignment - 1)) == 0)) {
auto &objects = std::get<omp::ObjectList>(clause.t);
if (!objects.empty())
genObjectList(objects, converter, alignedVars);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index d214d222e7c90..c19abea5becf8 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3244,9 +3244,14 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Aligned &x) {
x.v, llvm::omp::OMPC_aligned, GetContext().clauseSource, context_)) {
auto &modifiers{OmpGetModifiers(x.v)};
if (auto *align{OmpGetUniqueModifier<parser::OmpAlignment>(modifiers)}) {
- if (const auto &v{GetIntValue(align->v)}; !v || *v <= 0) {
+ const auto &v{GetIntValue(align->v)};
+ if (!v || *v <= 0) {
context_.Say(OmpGetModifierSource(modifiers, align),
"The alignment value should be a constant positive integer"_err_en_US);
+ } else if (((*v) & (*v - 1)) != 0) {
+ context_.Warn(common::UsageWarning::OpenMPUsage,
+ OmpGetModifierSource(modifiers, align),
+ "Alignment is not a power of 2, Aligned clause will be ignored"_warn_en_US);
}
}
}
diff --git a/flang/test/Lower/OpenMP/simd.f90 b/flang/test/Lower/OpenMP/simd.f90
index d815474b84b31..7655c786573e3 100644
--- a/flang/test/Lower/OpenMP/simd.f90
+++ b/flang/test/Lower/OpenMP/simd.f90
@@ -226,6 +226,23 @@ subroutine simdloop_aligned_allocatable()
end do
end subroutine
+subroutine aligned_non_power_of_two()
+ integer :: i
+ integer, allocatable :: A(:)
+ allocate(A(10))
+!CHECK: %[[A_PTR:.*]] = fir.alloca !fir.box<!fir.heap<!fir.array<?xi32>>> {bindc_name = "a",
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"}
+!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %[[A_PTR]] {fortran_attrs = #fir.var_attrs<allocatable>,
+!CHECK-SAME: uniq_name = "_QFaligned_non_power_of_twoEa"} :
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) ->
+!CHECK-SAME: (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
+!CHECK: omp.simd private
+ !$OMP SIMD ALIGNED(A:257)
+ do i = 1, 10
+ A(i) = i
+ end do
+end subroutine
+
!CHECK-LABEL: func @_QPsimd_with_nontemporal_clause
subroutine simd_with_nontemporal_clause(n)
!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFsimd_with_nontemporal_clauseEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
diff --git a/flang/test/Semantics/OpenMP/simd-aligned.f90 b/flang/test/Semantics/OpenMP/simd-aligned.f90
index 0a9f95833e22e..4c410a7c4631b 100644
--- a/flang/test/Semantics/OpenMP/simd-aligned.f90
+++ b/flang/test/Semantics/OpenMP/simd-aligned.f90
@@ -60,9 +60,16 @@ program omp_simd
!$omp end simd
!ERROR: 'd' in ALIGNED clause must be of type C_PTR, POINTER or ALLOCATABLE
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
!$omp simd aligned(d:100)
do i = 1, 100
d(i) = i
end do
+ !WARNING: Alignment is not a power of 2, Aligned clause will be ignored [-Wopen-mp-usage]
+ !$omp simd aligned(b:65)
+ do i = 1, 100
+ b(i) = i
+ end do
+
end program omp_simd
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 9f18199c75b4b..48010c7ec1a14 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2893,6 +2893,11 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
alignment = builder.getInt64(intAttr.getInt());
assert(ty->isPointerTy() && "Invalid type for aligned variable");
assert(alignment && "Invalid alignment value");
+ // Check if the alignment value is not a power of 2. If so, skip emitting
+ // alignment.
+ if (!intAttr.getValue().isPowerOf2())
+ continue;
+
auto curInsert = builder.saveIP();
builder.SetInsertPoint(sourceBlock);
llvmVal = builder.CreateLoad(ty, llvmVal);
diff --git a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
index 234604e4b664a..fd19ca15f5df0 100644
--- a/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-simd-aligned.mlir
@@ -58,3 +58,21 @@ llvm.func @_QPsimd_aligned_allocatable() {
}
llvm.return
}
+
+//CHECK-LABEL: define void @_QPsimd_aligned_non_power_of_two() {
+//CHECK: %[[A_ADDR:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, i64 1, align 8
+//CHECK-NOT: call void @llvm.assume(i1 true) [ "align"(ptr %{{.*}}, i64 256) ]
+llvm.func @_QPsimd_aligned_non_power_of_two() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)> {bindc_name = "a"} : (i64) -> !llvm.ptr
+ %2 = llvm.mlir.constant(1 : i32) : i32
+ %3 = llvm.mlir.constant(10 : i32) : i32
+ %4 = llvm.mlir.constant(1 : i32) : i32
+ omp.simd aligned(%1 : !llvm.ptr -> 257 : i64) {
+ omp.loop_nest (%arg0) : i32 = (%2) to (%3) inclusive step (%4) {
+ omp.yield
+ }
+ }
+ llvm.return
+}
+
|
| // Check if the alignment value is not a power of 2. If so, skip emitting | ||
| // alignment. | ||
| if (!intAttr.getValue().isPowerOf2()) | ||
| continue; |
There was a problem hiding this comment.
I am just thinking out loud what we really want to do here.
Should we
- Ignore this clause without doing anything
- Ignore this clause with a warning
- Error out
Previous situation was 3, this patch changes it to 2. I am assuming Option 1 is incorrect IR?
In the current implementation for non-Flang users of OpenMP MLIR, this would be a surprise. Ideally, we should emit the warning at the point the clause is ignored. I guess this might mean that the warning cannot be controlled with a Flang frontend flag. WDYT?
There was a problem hiding this comment.
Thankyou for reviewing the patch.
yes, I agree with you. As you mentioned, it would be more accurate to place the warning in openmp mlir instead of handling it in Flang frontend.
|
|
||
| //CHECK-LABEL: define void @_QPsimd_aligned_non_power_of_two() { | ||
| //CHECK: %[[A_ADDR:.*]] = alloca { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, i64 1, align 8 | ||
| //CHECK-NOT: call void @llvm.assume(i1 true) [ "align"(ptr %{{.*}}, i64 256) ] |
There was a problem hiding this comment.
Do you mean i64 256 instead of i64 257 here?
There was a problem hiding this comment.
Apologies, I made a mistake here.
yes, It should be i64 257. I supposed to add a check to verify that it is not emitting the call to llvm.assume(i1 true) [ "align"(ptr %{{.*}}, i64 257) ] but mistakenly written it as i64 256
…lect::verifyAlignedClause()
|
Hi @kiranchandramohan , |
Since the OpenMP standard does not disallow it, it probably only makes sense to give this warning while translating to LLVM IR. So I would prefer the place where it is ignored as the place where the warning is given, unless there are other infrastructural reasons. |
… during LLVM IR translation of aligned clause
I have updated the patch to emit a warning during LLVM IR translation. The aligned clause is now preserved throughout all lowering stages and only ignored during the LLVM IR translation if the alignment is not a power of 2. |
| emitWarning(simdOp->getLoc()) | ||
| << "The specified alignment value, " << intAttr.getInt() | ||
| << " is not a power of two and will be ignored"; |
There was a problem hiding this comment.
Could you add a test for this warning as well? There are similar tests in mlir/test/Target/LLVMIR/openmp-todo.mlir.
There was a problem hiding this comment.
Thankyou for pointing out the file. I have added a testcase for this scenario.
kaviya2510
changed the title
|
Ping for review! |
There was a problem hiding this comment.
Thanks for the fix.
I'm not sure about this approach. The warnings from the mlir to llvmir conversion will not be formatted as proper diagnostics and will not be controllable using the usual flags influencing other diagnostics.
Things like this should be done in flang semantic checks to ensure consistency.
It is good to catch this for other users of the OpenMP MLIR dialect too. In this case I would personally update the operation verifiers to not allow these unsupported alignment values.
|
Hi @tblah , Later, I revised the implementation because OpenMP specification does not explicitly state that non-power-of-two alignment values should be ignored. With that in mind, I handled emitting the warning in MLIR to LLVM IR translation phase. @kiranchandramohan , I hope this aligns with your suggestion. Kindly share your thoughts if you have any additional suggestions. Thankyou. |
This is a concern for me as well. But my assumption was different parts of the compiler will be providing warnings and it is for the driver to format them and control appropriately. This might not be the situation now.
The issue I had with these two points was that this warning is not part of the OpenMP standard. Other compilers do not seem to be emitting the warning. Since @tblah seems to have a strong opinion here. May be we should discuss it with others as well and then take a decision. Either way please go with @tblah's and other maintainers (@kparzysz @skatrak) direction. |
|
Likewise, if others would rather keep it here that's okay with me. |
|
Having a non-power-of-2 alignment sounds unusual to me. This may be unintended, let me ask the OpenMP ARB about this... |
Sure, please check and let me know the details. Thankyou! |
Issue:
When SIMD aligned clause has a alignment value which is not a power of 2, compiler crashes with error Assertion (alignment & (alignment - 1)) == 0 && "alignment is not power of 2"
Fix:
According to LLVM Language Reference manual [link], the alignment value may be non-power-of-two. In that case, the pointer value must be a null pointer otherwise the behavior is undefined.
So instead of emitting
llvm.assumeintrinsic function with a null pointer having the specified alignment, modified the implementation which ignores the aligned clause which has an alignment value which is not a power of 2.It fixes the issue #149458