[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added inline comments.
Herald added a project: All.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);

Meinersbur wrote:
> peixin wrote:
> > Nit: integer -> 64-bit integer?
> not necessarily, we do not require a specific integer size. For instance, 
> `__kmpc_for_static_init_4u` takes a 32-bit integer. It is up to the applyXYZ 
> function to zext/trunc it when necessary.
Got it. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);

peixin wrote:
> Nit: integer -> 64-bit integer?
not necessarily, we do not require a specific integer size. For instance, 
`__kmpc_for_static_init_4u` takes a 32-bit integer. It is up to the applyXYZ 
function to zext/trunc it when necessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Except for three nits. LGTM.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505
+  /// valid in the condition block (i.e., defined in the preheader) and is
+  /// interpreted as an unsigned integer.
+  void setTripCount(Value *TripCount);

Nit: integer -> 64-bit integer?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1676
+  Value *SrcLoc = getOrCreateIdent(getOrCreateSrcLocStr(DL));
+  Value *ThreadNum = getOrCreateThreadID(SrcLoc);
+  Constant *SchedulingType = ConstantInt::get(

peixin wrote:
> Can you move "Value *ThreadNum = getOrCreateThreadID(SrcLoc);" after 
> "Builder.CreateStore(One, PStride);" in order that the 
> "kmpc_global_thread_num" call is right before the "kmpc_static_init" call to 
> keep consistence with others?
This comment is not addressed.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1765
+  switch (SchedKind) {
+  case llvm::omp::ScheduleKind ::OMP_SCHEDULE_Default:
+assert(!ChunkSize && "No chunk size with default schedule (which for clang 
"

peixin wrote:
> Please remove the space between "ScheduleKind" and "OMP_SCHEDULE_Default"? 
> Also for the following switch cases.
The extra space is not removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Thanks for the fix. The fix of off-by-one issue looks ok to me. Will continue 
reviewing other parts in one week due to the Spring Festival in China.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@peixin Thanks for testing edge cases. You hit multiple issues:

1. Chunksize was i32, combining it with a i64 induction variable caused an 
error. Fixed the latest update of this patch.
2. OpenMPIRBuilder currently doesn't work really with exceptions. See D115216 
 for a start of a discussion with `#pragma 
omp parallel`. Support for irregular exits (exceptions, cancellation, 
destructors) out of CanonicalLoopInfo is what I was working on recently. Use 
`-fno-exceptions` to work around.
3. There is an off-by-one error that I already fixed in my development branch. 
Upstream patch here: D118542 

Result with these fixes for me is:

  lb: 18446744073709551615
  ub: 1844674407370955161
  step: 1844674407370955161
  18446744073709551615
  16602069666338596454
  14757395258967641293
  12912720851596686132
  11068046444225730971
  9223372036854775810
  7378697629483820649
  5534023222112865488
  3689348814741910327
  1844674407370955166

Note that this does involve `__kmpc_doacross_init` code in libomp you 
pointed-to in D116292 . This uses 
`__kmpc_for_static_init` calls of which there are 4 variants for 
(signed/unsigned x 32/64 bits). To do `__kmpc_doacross_init` correctly, it 
would also need at least have variants for signed/unsigned (or one working 
internally with signed 128 bits).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

When I investigated the edge cases you mentioned in D116292 
. Found one unsupported case as follows

  #include 
  #include 
  using namespace std;
  
  void func(unsigned long long lb, unsigned long long ub, unsigned long long 
step) {
unsigned long long i;
#pragma omp for schedule(static, 1)
for (i = lb; i > ub; i -= step) {
  cout << i << endl;
}
  }
  
  int main() {
unsigned long long lb, ub, step;
lb = ULLONG_MAX;
ub = ULLONG_MAX / 10;
step = ULLONG_MAX / 10;
cout << "lb: " << lb << endl;
cout << "ub: " << ub << endl;
cout << "step: " << step << endl;
  
func(lb, ub, step);
  
cout << endl;
return 0;
  }

  $ clang++ temp.cpp -fopenmp && ./a.out
  lb: 18446744073709551615
  ub: 1844674407370955161
  step: 1844674407370955161
  18446744073709551615
  16602069666338596454
  14757395258967641293
  12912720851596686132
  11068046444225730971
  9223372036854775810
  7378697629483820649
  5534023222112865488
  3689348814741910327
  1844674407370955166
  $ clang++ temp.cpp -fopenmp -fopenmp-enable-irbuilder
  clang-14: 
/home/qpx/compilers/llvm-community/static-chunk-codegen/llvm-project/llvm/lib/IR/Instructions.cpp:506:
 void llvm::CallInst::init(llvm::FunctionType*, llvm::Value*, 
llvm::ArrayRef, 
llvm::ArrayRef >, const llvm::Twine&): 
Assertion `(i >= FTy->getNumParams() || FTy->getParamType(i) == 
Args[i]->getType()) && "Calling a function with a bad signature!"' failed.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:

This is also for `schedule(static)`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Can you rebase this? I cannot apply this patch on current main branch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

BTW, please rebase on main. There is one conflict about function 
`getOrCreateSrcLocStr`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.
Herald added a subscriber: awarzynski.

@Meinersbur Here is the c++ code test. Without the chunk size specified, the 
running result using OMPIRBuilder is correct.

  #include
  
  int main() {
int i, N = 10;
float x = 0.0;
  
#pragma omp for schedule(static, 2)
for(i = 3; i <= N; i++) {
  x = x + i;
}
  
std::cout << "x = " << x << std::endl;
  
return 0;
  }

  $ clang++ chunk-1.cpp -fopenmp -fopenmp-enable-irbuilder && ./a.out
  x = 7
  $ clang++ chunk-1.cpp -fopenmp && ./a.out
  x = 52


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

Can you check the following example by applying this patch on fir-dev?

  program main
integer :: i, N = 10
real :: x = 0
  
!$omp do schedule(static, 2)
do i = 3, N
  x = x + i
end do
!$omp end do
  
print *, x
  end

Test running result:

  $ gfortran test.f90 -fopenmp && ./a.out
 52.000
  $ bbc -fopenmp -emit-fir test.f90
  $ tco test.mlir -o test.ll
  $ clang++ -lFortran_main -lFortranRuntime -lFortranDecimal -lomp -o a.out 
test.ll
  $ ./a.out
   7.

When you change "schedule(static, 2)" into "schedule(static, 1)", the running 
result is 3.0.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1676
+  Value *SrcLoc = getOrCreateIdent(getOrCreateSrcLocStr(DL));
+  Value *ThreadNum = getOrCreateThreadID(SrcLoc);
+  Constant *SchedulingType = ConstantInt::get(

Can you move "Value *ThreadNum = getOrCreateThreadID(SrcLoc);" after 
"Builder.CreateStore(One, PStride);" in order that the "kmpc_global_thread_num" 
call is right before the "kmpc_static_init" call to keep consistence with 
others?



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1765
+  switch (SchedKind) {
+  case llvm::omp::ScheduleKind ::OMP_SCHEDULE_Default:
+assert(!ChunkSize && "No chunk size with default schedule (which for clang 
"

Please remove the space between "ScheduleKind" and "OMP_SCHEDULE_Default"? Also 
for the following switch cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D114413#3166979 , @Meinersbur 
wrote:

> In D114413#3160044 , @peixin wrote:
>
>> In D114413#3159095 , @Meinersbur 
>> wrote:
>>
>>> In D114413#3154936 , @peixin 
>>> wrote:
>>>
 
>>
>> `getPreheader()` was in `OMPIRBuilder.cpp` before you rebase in your last 
>> update here. That's why I let you rebase since I failed to git apply your 
>> last patch in main branch. It's not important now. Please forget about that.
>
> D114368  (which this patch depends on) 
> moves `getPreheder()` to the `.cpp` files (because it has become more than a 
> simple getter)

Thanks a lot. Now I get it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D114413#3160044 , @peixin wrote:

> In D114413#3159095 , @Meinersbur 
> wrote:
>
>> In D114413#3154936 , @peixin wrote:
>>
>>> 
>
> `getPreheader()` was in `OMPIRBuilder.cpp` before you rebase in your last 
> update here. That's why I let you rebase since I failed to git apply your 
> last patch in main branch. It's not important now. Please forget about that.

D114368  (which this patch depends on) moves 
`getPreheder()` to the `.cpp` files (because it has become more than a simple 
getter)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

In D114413#3159095 , @Meinersbur 
wrote:

> In D114413#3154936 , @peixin wrote:
>
>> @Meinersbur Please rebase on main. The function "getPreheader()" was moved 
>> into OMPIRBuilder.h.
>
> I rebased, but I am not sure what you are referring to. `getPreheader()` 
> always was in `OMPIRBuilder.h`

`getPreheader()` was in `OMPIRBuilder.cpp` before you rebase in your last 
update here. That's why I let you rebase since I failed to git apply your last 
patch in main branch. It's not important now. Please forget about that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D114413#3154936 , @peixin wrote:

> @Meinersbur Please rebase on main. The function "getPreheader()" was moved 
> into OMPIRBuilder.h.

I rebased, but I am not sure what you are referring to. `getPreheader()` always 
was in `OMPIRBuilder.h`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

[Peixin Qiao via Phabricator via cfe-commits]

peixin added a comment.

@Meinersbur Please rebase on main. The function "getPreheader()" was moved into 
OMPIRBuilder.h.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114413/new/

https://reviews.llvm.org/D114413

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits