Skip to content

Conversation

khaki3
Copy link
Contributor

@khaki3 khaki3 commented Aug 27, 2025

We currently see the misuse of the upperbound operand for the acc::DataBoundsOp generation in generateSeqTyAccBounds. This PR keeps setting lowerbound to zero for all cases and adjusts upperbound to be extent - 1.

@khaki3 khaki3 requested a review from razvanlupusoru August 27, 2025 17:58
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir openacc labels Aug 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2025

@llvm/pr-subscribers-flang-fir-hlfir

Author: None (khaki3)

Changes

We currently see the misuse of the startIdx operand for the acc::DataBoundsOp generation in generateSeqTyAccBounds. This PR sets startIdx to one for all cases and instead adjusts both lowerbound and upperbound.


Full diff: https://github.com/llvm/llvm-project/pull/155666.diff

2 Files Affected:

  • (modified) flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp (+3-5)
  • (modified) flang/test/Fir/OpenACC/openacc-mappable.fir (+2-2)
diff --git a/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp b/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
index 5b6d904fb0d59..9b4db4900258b 100644
--- a/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
+++ b/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
@@ -243,8 +243,6 @@ generateSeqTyAccBounds(fir::SequenceType seqType, mlir::Value var,
               mlir::dyn_cast_if_present<fir::ShapeOp>(shape.getDefiningOp())) {
         mlir::Value cummulativeExtent = one;
         for (auto extent : shapeOp.getExtents()) {
-          mlir::Value upperbound =
-              mlir::arith::SubIOp::create(builder, loc, extent, one);
           mlir::Value stride = one;
           if (strideIncludeLowerExtent) {
             stride = cummulativeExtent;
@@ -254,7 +252,7 @@ generateSeqTyAccBounds(fir::SequenceType seqType, mlir::Value var,
           auto accBound = mlir::acc::DataBoundsOp::create(
               builder, loc,
               mlir::acc::DataBoundsType::get(builder.getContext()),
-              /*lowerbound=*/zero, /*upperbound=*/upperbound,
+              /*lowerbound=*/one, /*upperbound=*/extent,
               /*extent=*/extent, /*stride=*/stride, /*strideInBytes=*/false,
               /*startIdx=*/one);
           accBounds.push_back(accBound);
@@ -282,9 +280,9 @@ generateSeqTyAccBounds(fir::SequenceType seqType, mlir::Value var,
             auto accBound = mlir::acc::DataBoundsOp::create(
                 builder, loc,
                 mlir::acc::DataBoundsType::get(builder.getContext()),
-                /*lowerbound=*/zero, /*upperbound=*/upperbound,
+                /*lowerbound=*/lowerbound, /*upperbound=*/upperbound,
                 /*extent=*/extent, /*stride=*/stride, /*strideInBytes=*/false,
-                /*startIdx=*/lowerbound);
+                /*startIdx=*/one);
             accBounds.push_back(accBound);
           }
         }
diff --git a/flang/test/Fir/OpenACC/openacc-mappable.fir b/flang/test/Fir/OpenACC/openacc-mappable.fir
index 71576f4b71075..3535bfaae3c30 100644
--- a/flang/test/Fir/OpenACC/openacc-mappable.fir
+++ b/flang/test/Fir/OpenACC/openacc-mappable.fir
@@ -62,12 +62,12 @@ module attributes {dlti.dl_spec = #dlti.dl_spec<f16 = dense<16> : vector<2xi64>,
   // CHECK: Visiting: %{{.*}} = acc.copyin varPtr(%{{.*}} : !fir.ref<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>> {name = "arr1", structured = false}
   // CHECK: Pointer-like and Mappable: !fir.ref<!fir.array<?xf32>>
   // CHECK: Type category: array
-  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c0{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c1{{.*}} : index)
+  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c1{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c1{{.*}} : index)
 
   // CHECK: Visiting: %{{.*}} = acc.copyin varPtr(%{{.*}} : !fir.ref<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>> {name = "arr2", structured = false}
   // CHECK: Pointer-like and Mappable: !fir.ref<!fir.array<?xf32>>
   // CHECK: Type category: array
-  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c0{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c2{{.*}} : index)
+  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c2{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c1{{.*}} : index)
 
   // CHECK: Visiting: %{{.*}} = acc.copyin varPtr(%{{.*}} : !fir.ref<!fir.array<10xf32>>) -> !fir.ref<!fir.array<10xf32>> {name = "arr3", structured = false}
   // CHECK: Pointer-like and Mappable: !fir.ref<!fir.array<10xf32>>

@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2025

@llvm/pr-subscribers-openacc

Author: None (khaki3)

Changes

We currently see the misuse of the startIdx operand for the acc::DataBoundsOp generation in generateSeqTyAccBounds. This PR sets startIdx to one for all cases and instead adjusts both lowerbound and upperbound.


Full diff: https://github.com/llvm/llvm-project/pull/155666.diff

2 Files Affected:

  • (modified) flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp (+3-5)
  • (modified) flang/test/Fir/OpenACC/openacc-mappable.fir (+2-2)
diff --git a/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp b/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
index 5b6d904fb0d59..9b4db4900258b 100644
--- a/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
+++ b/flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
@@ -243,8 +243,6 @@ generateSeqTyAccBounds(fir::SequenceType seqType, mlir::Value var,
               mlir::dyn_cast_if_present<fir::ShapeOp>(shape.getDefiningOp())) {
         mlir::Value cummulativeExtent = one;
         for (auto extent : shapeOp.getExtents()) {
-          mlir::Value upperbound =
-              mlir::arith::SubIOp::create(builder, loc, extent, one);
           mlir::Value stride = one;
           if (strideIncludeLowerExtent) {
             stride = cummulativeExtent;
@@ -254,7 +252,7 @@ generateSeqTyAccBounds(fir::SequenceType seqType, mlir::Value var,
           auto accBound = mlir::acc::DataBoundsOp::create(
               builder, loc,
               mlir::acc::DataBoundsType::get(builder.getContext()),
-              /*lowerbound=*/zero, /*upperbound=*/upperbound,
+              /*lowerbound=*/one, /*upperbound=*/extent,
               /*extent=*/extent, /*stride=*/stride, /*strideInBytes=*/false,
               /*startIdx=*/one);
           accBounds.push_back(accBound);
@@ -282,9 +280,9 @@ generateSeqTyAccBounds(fir::SequenceType seqType, mlir::Value var,
             auto accBound = mlir::acc::DataBoundsOp::create(
                 builder, loc,
                 mlir::acc::DataBoundsType::get(builder.getContext()),
-                /*lowerbound=*/zero, /*upperbound=*/upperbound,
+                /*lowerbound=*/lowerbound, /*upperbound=*/upperbound,
                 /*extent=*/extent, /*stride=*/stride, /*strideInBytes=*/false,
-                /*startIdx=*/lowerbound);
+                /*startIdx=*/one);
             accBounds.push_back(accBound);
           }
         }
diff --git a/flang/test/Fir/OpenACC/openacc-mappable.fir b/flang/test/Fir/OpenACC/openacc-mappable.fir
index 71576f4b71075..3535bfaae3c30 100644
--- a/flang/test/Fir/OpenACC/openacc-mappable.fir
+++ b/flang/test/Fir/OpenACC/openacc-mappable.fir
@@ -62,12 +62,12 @@ module attributes {dlti.dl_spec = #dlti.dl_spec<f16 = dense<16> : vector<2xi64>,
   // CHECK: Visiting: %{{.*}} = acc.copyin varPtr(%{{.*}} : !fir.ref<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>> {name = "arr1", structured = false}
   // CHECK: Pointer-like and Mappable: !fir.ref<!fir.array<?xf32>>
   // CHECK: Type category: array
-  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c0{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c1{{.*}} : index)
+  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c1{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c1{{.*}} : index)
 
   // CHECK: Visiting: %{{.*}} = acc.copyin varPtr(%{{.*}} : !fir.ref<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>> {name = "arr2", structured = false}
   // CHECK: Pointer-like and Mappable: !fir.ref<!fir.array<?xf32>>
   // CHECK: Type category: array
-  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c0{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c2{{.*}} : index)
+  // CHECK: Bound[0]: %{{.*}} = acc.bounds lowerbound(%c2{{.*}} : index) upperbound(%{{.*}} : index) extent(%{{.*}} : index) stride(%c1{{.*}} : index) startIdx(%c1{{.*}} : index)
 
   // CHECK: Visiting: %{{.*}} = acc.copyin varPtr(%{{.*}} : !fir.ref<!fir.array<10xf32>>) -> !fir.ref<!fir.array<10xf32>> {name = "arr3", structured = false}
   // CHECK: Pointer-like and Mappable: !fir.ref<!fir.array<10xf32>>

@khaki3 khaki3 closed this Aug 27, 2025
@khaki3 khaki3 reopened this Aug 27, 2025
@khaki3
Copy link
Contributor Author

khaki3 commented Aug 27, 2025

@razvanlupusoru, thanks for your review. I've updated the fix.

Copy link
Contributor

@razvanlupusoru razvanlupusoru left a comment

Choose a reason for hiding this comment

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

Nice catch since I missed that I actually used zero for the lowerbound. Please fix the test and I can approve :) Thank you.

Copy link
Contributor

@razvanlupusoru razvanlupusoru left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you!

@khaki3 khaki3 merged commit 1581a4b into llvm:main Aug 28, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category openacc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants