[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2172748 , @protze.joachim 
wrote:

> I carefully made sure, that the freshly built clang was used to execute the 
> test. I opened https://bugs.llvm.org/show_bug.cgi?id=46836 to track the issue 
> and made it release blocker.


The question is if you picked up a fresly build device runtime as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim reopened this revision.
protze.joachim added a comment.
This revision is now accepted and ready to land.

I carefully made sure, that the freshly built clang was used to execute the 
test. I opened https://bugs.llvm.org/show_bug.cgi?id=46836 to track the issue 
and made it release blocker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Joachim Protze via Phabricator via cfe-commits]

protze.joachim added a comment.

This patch breaks compilation of previously working code.

I added the following to 
`openmp/libomptarget/test/offloading/offloading_success.c`:

  +// RUN: %libomptarget-compile-run-and-check-nvptx64-nvidia-cuda

which results in

  # command stderr:
  ptxas offloading_success-openmp-nvptx64-nvidia-cuda.s, line 173; error   : 
Call has wrong number of parameters
  ptxas fatal   : Ptx assembly aborted due to errors
  clang-12: error: ptxas command failed with exit code 255 (use -v to see 
invocation)

The file `offloading_success-openmp-nvptx64-nvidia-cuda.s` contains:

  .func  (.param .b32 func_retval0) __kmpc_kernel_parallel
  (
  .param .b64 __kmpc_kernel_parallel_param_0,
  .param .b32 __kmpc_kernel_parallel_param_1
  )
  ;

For the clang 11 release, we should either fix the codegen or revert this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc98699582a63: [OpenMP][NFC] Remove unused (always fixed) 
arguments (authored by jdoerfert).

Changed prior to commit:
  https://reviews.llvm.org/D83268?vs=275871=277218#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268

Files:
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/test/OpenMP/nvptx_data_sharing.cpp
  clang/test/OpenMP/nvptx_parallel_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/nvptx_target_teams_codegen.cpp
  clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  openmp/libomptarget/deviceRTLs/common/src/parallel.cu
  openmp/libomptarget/deviceRTLs/interface.h

Index: openmp/libomptarget/deviceRTLs/interface.h
===
--- openmp/libomptarget/deviceRTLs/interface.h
+++ openmp/libomptarget/deviceRTLs/interface.h
@@ -424,10 +424,8 @@
 EXTERN void __kmpc_spmd_kernel_init(int ThreadLimit, int16_t RequiresOMPRuntime,
 int16_t RequiresDataSharing);
 EXTERN void __kmpc_spmd_kernel_deinit_v2(int16_t RequiresOMPRuntime);
-EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn,
-   int16_t IsOMPRuntimeInitialized);
-EXTERN bool __kmpc_kernel_parallel(void **WorkFn,
-   int16_t IsOMPRuntimeInitialized);
+EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn);
+EXTERN bool __kmpc_kernel_parallel(void **WorkFn);
 EXTERN void __kmpc_kernel_end_parallel();
 
 EXTERN void __kmpc_data_sharing_init_stack();
Index: openmp/libomptarget/deviceRTLs/common/src/parallel.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/parallel.cu
+++ openmp/libomptarget/deviceRTLs/common/src/parallel.cu
@@ -72,10 +72,8 @@
 }
 
 // This routine is always called by the team master..
-EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn,
-   int16_t IsOMPRuntimeInitialized) {
+EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn) {
   PRINT0(LD_IO, "call to __kmpc_kernel_prepare_parallel\n");
-  ASSERT0(LT_FUSSY, IsOMPRuntimeInitialized, "Expected initialized runtime.");
 
   omptarget_nvptx_workFn = WorkFn;
 
@@ -120,12 +118,9 @@
 // returns True if this thread is active, else False.
 //
 // Only the worker threads call this routine.
-EXTERN bool __kmpc_kernel_parallel(void **WorkFn,
-   int16_t IsOMPRuntimeInitialized) {
+EXTERN bool __kmpc_kernel_parallel(void **WorkFn) {
   PRINT0(LD_IO | LD_PAR, "call to __kmpc_kernel_parallel\n");
 
-  ASSERT0(LT_FUSSY, IsOMPRuntimeInitialized, "Expected initialized runtime.");
-
   // Work function and arguments for L1 parallel region.
   *WorkFn = omptarget_nvptx_workFn;
 
Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -584,6 +584,11 @@
 __OMP_RTL(__kmpc_task_allow_completion_event, false, VoidPtr, IdentPtr,
   /* Int */ Int32, /* kmp_task_t */ VoidPtr)
 
+/// Note that device runtime functions (in the following) do not necessarily
+/// need attributes as we expect to see the definitions.
+__OMP_RTL(__kmpc_kernel_parallel, false, Int1, VoidPtrPtr)
+__OMP_RTL(__kmpc_kernel_prepare_parallel, false, Void, VoidPtr)
+
 __OMP_RTL(__last, false, Void, )
 
 #undef __OMP_RTL
Index: clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
===
--- clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
+++ clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
@@ -88,7 +88,7 @@
   // CHECK: [[I_ADDR:%.+]] = getelementptr inbounds [[GLOB_TY]], [[GLOB_TY]]* [[RD]], i32 0, i32 0
   //
   // CHECK: call void @__kmpc_for_static_init_4(
-  // CHECK: call void @__kmpc_kernel_prepare_parallel(i8* bitcast (void (i16, i32)* @{{.+}} to i8*), i16 1)
+  // CHECK: call void @__kmpc_kernel_prepare_parallel(i8* bitcast (void (i16, i32)* @{{.+}} to i8*))
   // CHECK: call void @__kmpc_begin_sharing_variables(i8*** [[SHARED_VARS_PTR:%.+]], i{{64|32}} 1)
   // CHECK: [[SHARED_VARS_BUF:%.+]] = load i8**, i8*** [[SHARED_VARS_PTR]],
   // CHECK: [[VARS_BUF:%.+]] = getelementptr inbounds i8*, i8** [[SHARED_VARS_BUF]], i{{64|32}} 0
Index: clang/test/OpenMP/nvptx_target_teams_codegen.cpp
===
--- clang/test/OpenMP/nvptx_target_teams_codegen.cpp
+++ clang/test/OpenMP/nvptx_target_teams_codegen.cpp
@@ -68,7 +68,7 @@
   //
   // CHECK: [[AWAIT_WORK]]
   // CHECK: call void @__kmpc_barrier_simple_spmd(%struct.ident_t* null, i32 0)
- 

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

__kmpc_spmd_kernel_init is always called with RequiresDataSharing == 0
Specifically, it's only called from clang, and emitSPMDEntryHeader 
unconditionally passes zero to it

I.e. I think there's more stuff that can be cleaned up in the theme of the 
above, suggest in later patches


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2136060 , @ABataev wrote:

> Better to ask the users. Maybe, send an RFC to openmp-devs?


Sure: http://lists.llvm.org/pipermail/openmp-dev/2020-July/003531.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

In D83268#2135930 , @JonChesterfield 
wrote:

> Aside from the API stability concern this looks uncontentious. Removes dead 
> arguments, generally makes things simpler. Thus LGTM.
>
> @Hahnfeld @ABataev - are you sufficiently persuaded that preserving the 
> current interface is not worth the development cost?


I'm neither, and I've long argued that being able to build the OpenMP 
runtime(s) without Clang trunk is an important use case. These arguments have 
gone largely unheard, so I'll not join this discussion once more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2136055 , @ABataev wrote:

> In D83268#2136054 , @jdoerfert wrote:
>
> > In D83268#2136029 , @ABataev wrote:
> >
> > > `llvm-project/openmp/libomptarget`
> >
> >
> > Please use more words.
>
>
> libomptarget is part of libomp


As mentioned before, no it is not.

Despite the similarity in name, libomp and libomptarget are distinct libraries, 
this was a conscious design choice.
FWIW, this patch does *not* modify libomptarget either. This modifies *the 
device runtime*, aka. libomptarget-nvptx-sm_XXX.bc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2136049 , @jdoerfert wrote:

> In D83268#2136031 , @ABataev wrote:
>
> > There is still compatibility between clang10 and clang11. Or they are 
> > incompatible in LLVM IR level?
>
>
> That is the point. They might or might not be, right? There is no guarantee 
> they are.


Better to ask the users. Maybe, send an RFC to openmp-devs?

> 
> 
>> Also, there was a mode (I don't remember if it was removed or not) where the 
>> runtime library could be linked as `.a` library, without LLVM IR inlining.
> 
> That mode is deprecated.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2136054 , @jdoerfert wrote:

> In D83268#2136029 , @ABataev wrote:
>
> > `llvm-project/openmp/libomptarget`
>
>
> Please use more words.


libomptarget is part of libomp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2136029 , @ABataev wrote:

> `llvm-project/openmp/libomptarget`


Please use more words.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2136031 , @ABataev wrote:

> There is still compatibility between clang10 and clang11. Or they are 
> incompatible in LLVM IR level?


That is the point. They might or might not be, right? There is no guarantee 
they are.

> Also, there was a mode (I don't remember if it was removed or not) where the 
> runtime library could be linked as `.a` library, without LLVM IR inlining.

That mode is deprecated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2135951 , @jdoerfert wrote:

> > I don't think gcc can be using this runtime library for nvptx.
>
> Yes, and: We are (going to) use clang specific intrinsics to avoid CUDA 
> (soon).
>
> > Use of the new library with the previous version of the compiler.
>
> Except that you cannot generally expect this to work. In our supported use 
> case the library is kept as bitcode (LLVM-IR). Bitcode is not backward 
> compatible. An old toolchain (clang, llvm-link, ...) cannot be fed new IR and 
> be expected to work. So, we are already not able to give a stability 
> guarantee here, why pretend we do. The bitcode runtime has to be kept in-sync 
> with the toolchain.


There is still compatibility between clang10 and clang11. Or they are 
incompatible in LLVM IR level? Also, there was a mode (I don't remember if it 
was removed or not) where the runtime library could be linked as `.a` library, 
without LLVM IR inlining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2136021 , @jdoerfert wrote:

> In D83268#2135988 , @ABataev wrote:
>
> > In D83268#2135955 , @jdoerfert 
> > wrote:
> >
> > > > Especially taking into account, that libomp can be built separately.
> > >
> > > This is *not* affecting libomp in any way.
> >
> >
> > libomptarget and device runtimes are part of libomp. If you're going to 
> > remove some params, you'll need to modify the runtime functions too, no?
>
>
> No they are not.


`llvm-project/openmp/libomptarget`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2135988 , @ABataev wrote:

> In D83268#2135955 , @jdoerfert wrote:
>
> > > Especially taking into account, that libomp can be built separately.
> >
> > This is *not* affecting libomp in any way.
>
>
> libomptarget and device runtimes are part of libomp. If you're going to 
> remove some params, you'll need to modify the runtime functions too, no?


No they are not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2135989 , @ABataev wrote:

> In D83268#2135954 , @JonChesterfield 
> wrote:
>
> > What can libomp be built by separately? Nvcc and gcc don't use this 
> > runtime. That leaves us with downstream proprietary compilers derived from 
> > clang that are already stuck carrying extensive compatibility patches and 
> > usually ship as one large toolchain blob which only needs to be internally 
> > self consistent.
>
>
> Answered already: the previous version of the compiler with the new version 
> of the runtime.


Still cannot be expected to work: https://reviews.llvm.org/D83268#2135951
Are there other use cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2135954 , @JonChesterfield 
wrote:

> In D83268#2135938 , @ABataev wrote:
>
> > > @Hahnfeld @ABataev - are you sufficiently persuaded that preserving the 
> > > current interface is not worth the development cost?
> >
> > No, I'm not. Long before that, we relied on the API stability and already 
> > have some runtime calls marked as deprecated. Especially taking into 
> > account, that libomp can be built separately.
>
>
> Yes, the existing v# naming and deprecated comments should also go.
>
> What can libomp be built by separately? Nvcc and gcc don't use this runtime. 
> That leaves us with downstream proprietary compilers derived from clang that 
> are already stuck carrying extensive compatibility patches and usually ship 
> as one large toolchain blob which only needs to be internally self consistent.


Answered already: the previous version of the compiler with the new version of 
the runtime.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2135955 , @jdoerfert wrote:

> > Especially taking into account, that libomp can be built separately.
>
> This is *not* affecting libomp in any way.


libomptarget and device runtimes are part of libomp. If you're going to remove 
some params, you'll need to modify the runtime functions too, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

> Especially taking into account, that libomp can be built separately.

This is *not* affecting libomp in any way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D83268#2135938 , @ABataev wrote:

> > @Hahnfeld @ABataev - are you sufficiently persuaded that preserving the 
> > current interface is not worth the development cost?
>
> No, I'm not. Long before that, we relied on the API stability and already 
> have some runtime calls marked as deprecated. Especially taking into account, 
> that libomp can be built separately.


Yes, the existing v# naming and deprecated comments should also go.

What can libomp be built by separately? Nvcc and gcc don't use this runtime. 
That leaves us with downstream proprietary compilers derived from clang that 
are already stuck carrying extensive compatibility patches and usually ship as 
one large toolchain blob which only needs to be internally self consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

> I don't think gcc can be using this runtime library for nvptx.

Yes, and: We are (going to) use clang specific intrinsics to avoid CUDA (soon).

> Use of the new library with the previous version of the compiler.

Except that you cannot generally expect this to work. In our supported use case 
the library is kept as bitcode (LLVM-IR). Bitcode is not backward compatible. 
An old toolchain (clang, llvm-link, ...) cannot be fed new IR and be expected 
to work. So, we are already not able to give a stability guarantee here, why 
pretend we do. The bitcode runtime has to be kept in-sync with the toolchain.




Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:42
-  /// *outlined_function, int16_t
-  /// IsOMPRuntimeInitialized);
   OMPRTL_NVPTX__kmpc_kernel_prepare_parallel,

JonChesterfield wrote:
> ABataev wrote:
> > I think, instead the optimizer can try to detect if the runtime library is 
> > used by the kernel and switch this flag to `0` if no runtime calls are used 
> > in the kernel. For non-SPMD mode in most cases, the runtime is required, 
> > but in some cases, it can be disabled.
> If we can detect that no runtime calls are used, we should be able to do 
> better than passing a different argument. E.g. delete some setup calls.
> 
> Failing that, if we want to pass an argument which says 'actually don't do 
> any work', it shouldn't be the same argument used to check whether the 
> runtime has been initialised.
We can detect all we want but switching it *does not have any effect*.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2135930 , @JonChesterfield 
wrote:

> Aside from the API stability concern this looks uncontentious. Removes dead 
> arguments, generally makes things simpler. Thus LGTM.
>
> @Hahnfeld @ABataev - are you sufficiently persuaded that preserving the 
> current interface is not worth the development cost?


No, I'm not. Long before that, we relied on the API stability and already have 
some runtime calls marked as deprecated. Especially taking into account, that 
libomp can be built separately.




Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:42
-  /// *outlined_function, int16_t
-  /// IsOMPRuntimeInitialized);
   OMPRTL_NVPTX__kmpc_kernel_prepare_parallel,

JonChesterfield wrote:
> ABataev wrote:
> > I think, instead the optimizer can try to detect if the runtime library is 
> > used by the kernel and switch this flag to `0` if no runtime calls are used 
> > in the kernel. For non-SPMD mode in most cases, the runtime is required, 
> > but in some cases, it can be disabled.
> If we can detect that no runtime calls are used, we should be able to do 
> better than passing a different argument. E.g. delete some setup calls.
> 
> Failing that, if we want to pass an argument which says 'actually don't do 
> any work', it shouldn't be the same argument used to check whether the 
> runtime has been initialised.
No, I don't think you can do this in all cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Aside from the API stability concern this looks uncontentious. Removes dead 
arguments, generally makes things simpler. Thus LGTM.

@Hahnfeld @ABataev - are you sufficiently persuaded that preserving the current 
interface is not worth the development cost?




Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:42
-  /// *outlined_function, int16_t
-  /// IsOMPRuntimeInitialized);
   OMPRTL_NVPTX__kmpc_kernel_prepare_parallel,

ABataev wrote:
> I think, instead the optimizer can try to detect if the runtime library is 
> used by the kernel and switch this flag to `0` if no runtime calls are used 
> in the kernel. For non-SPMD mode in most cases, the runtime is required, but 
> in some cases, it can be disabled.
If we can detect that no runtime calls are used, we should be able to do better 
than passing a different argument. E.g. delete some setup calls.

Failing that, if we want to pass an argument which says 'actually don't do any 
work', it shouldn't be the same argument used to check whether the runtime has 
been initialised.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I'm not sure we have a consensus on api stability. Usually llvm allows mixing 
libraries and compilers from different sources, e.g. the various libunwind or 
compiler-rt vs libgcc.  Libomptarget in general appears to be considered fixed 
and has external users (intel, maybe gcc).

The device runtime would be ill served by this default. This is the only openmp 
device runtime library which works with llvm. It's statically linked, usually 
as bitcode when performance is important. The code used to handle target 
offloading for nvptx is spread across the compiler and the runtime, probably 
not optimally.

I'm not familiar with the gcc-nvptx-openmp implementation. Reading through 
https://gcc.gnu.org/wiki/Offloading strongly suggests a totally independent 
implementation to this one. I don't think gcc can be using this runtime library 
for nvptx. It definitely doesn't for amdgcn. Proprietary compilers might be 
using this code, but we can have no duty of care to toolchains that use this 
code without telling us they're doing so.

Therefore the only backwards/forwards compatibility we can strive for is 
between different versions of clang and the device runtime. That seems 
potentially useful - one could use a release clang binary while working on the 
deviceRTL or vice versa. It's an expensive developer convenience though.

We would pay with things like the API rot fixed above. Introducing a faster 
lowering for an openmp construct would mean a redundant path through clang and 
some version checking to guess which runtime library we're targeting, which is 
not presently versioned. Likewise moving code between compiler and runtime 
becomes much more expensive to implement. Getting it wrong is inevitable given 
our test coverage.

I think the project is much better served by assuming that the runtime library 
used by clang is the one from the same hash in the monorepo. That leaves us 
free to fix debt and improve performance, at the price of needing to build 
clang from (near to) trunk while developing the rtl.

Perhaps we can embrace API stability later on, once we have things like 
versioning and a solid optimisation pipeline in place, especially if gcc wants 
to use the deviceRTL for nvptx. Now is too early.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:42
-  /// *outlined_function, int16_t
-  /// IsOMPRuntimeInitialized);
   OMPRTL_NVPTX__kmpc_kernel_prepare_parallel,

I think, instead the optimizer can try to detect if the runtime library is used 
by the kernel and switch this flag to `0` if no runtime calls are used in the 
kernel. For non-SPMD mode in most cases, the runtime is required, but in some 
cases, it can be disabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2135724 , @jdoerfert wrote:

> In D83268#2135081 , @Hahnfeld wrote:
>
> > This is definitely not NFC and breaks API compatibility (but apparently 
> > nobody cares anymore?).
>
>
> This is the device RTL. I am not aware we (want to) keep the API stable. If 
> we are, I'm not sure why:
>
> - Dynamic linking (among other things) is not really an option so people that 
> link against the device runtime (should) do so statically.
> - Linking against an old device runtime with a new clang seems unreasonable 
> to me. If you replace clang you must replace the static runtime as the new 
> clang might use new functions.
>
>
>
> In D83268#2135655 , @ABataev wrote:
>
> > In D83268#2135081 , @Hahnfeld 
> > wrote:
> >
> > > This is definitely not NFC and breaks API compatibility (but apparently 
> > > nobody cares anymore?).
> >
> >
> > +1. Better to introduce new entry points and mark these ones as deprecated.
>
>
> Same response as above. What is the use case here which we want to continue 
> to support?


Use of the new library with the previous version of the compiler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D83268#2135081 , @Hahnfeld wrote:

> This is definitely not NFC and breaks API compatibility (but apparently 
> nobody cares anymore?).


This is the device RTL. I am not aware we (want to) keep the API stable. If we 
are, I'm not sure why:

- Dynamic linking (among other things) is not really an option so people that 
link against the device runtime (should) do so statically.
- Linking against an old device runtime with a new clang seems unreasonable to 
me. If you replace clang you must replace the static runtime as the new clang 
might use new functions.



In D83268#2135655 , @ABataev wrote:

> In D83268#2135081 , @Hahnfeld wrote:
>
> > This is definitely not NFC and breaks API compatibility (but apparently 
> > nobody cares anymore?).
>
>
> +1. Better to introduce new entry points and mark these ones as deprecated.


Same response as above. What is the use case here which we want to continue to 
support?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83268#2135081 , @Hahnfeld wrote:

> This is definitely not NFC and breaks API compatibility (but apparently 
> nobody cares anymore?).


+1. Better to introduce new entry points and mark these ones as deprecated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

This is definitely not NFC and breaks ABI compatibility (but apparently nobody 
cares anymore).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83268



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

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: jhuber6, fghanim, JonChesterfield, grokos, 
AndreyChurbanov, ye-luo, tianshilei1992, ggeorgakoudis.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, guansong, bollu, 
yaxunl, jholewinski.
Herald added projects: clang, OpenMP, LLVM.

There are various runtime calls in the device runtime with unused, or
always fixed, arguments. This is bad for all sorts of reasons. Clean up
two before as we match them in OpenMPOpt now.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83268

Files:
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/test/OpenMP/nvptx_data_sharing.cpp
  clang/test/OpenMP/nvptx_parallel_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/nvptx_target_teams_codegen.cpp
  clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  openmp/libomptarget/deviceRTLs/common/src/parallel.cu
  openmp/libomptarget/deviceRTLs/interface.h

Index: openmp/libomptarget/deviceRTLs/interface.h
===
--- openmp/libomptarget/deviceRTLs/interface.h
+++ openmp/libomptarget/deviceRTLs/interface.h
@@ -475,10 +475,8 @@
 int16_t RequiresDataSharing);
 EXTERN __attribute__((deprecated)) void __kmpc_spmd_kernel_deinit();
 EXTERN void __kmpc_spmd_kernel_deinit_v2(int16_t RequiresOMPRuntime);
-EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn,
-   int16_t IsOMPRuntimeInitialized);
-EXTERN bool __kmpc_kernel_parallel(void **WorkFn,
-   int16_t IsOMPRuntimeInitialized);
+EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn);
+EXTERN bool __kmpc_kernel_parallel(void **WorkFn);
 EXTERN void __kmpc_kernel_end_parallel();
 EXTERN bool __kmpc_kernel_convergent_parallel(void *buffer,
   __kmpc_impl_lanemask_t Mask,
Index: openmp/libomptarget/deviceRTLs/common/src/parallel.cu
===
--- openmp/libomptarget/deviceRTLs/common/src/parallel.cu
+++ openmp/libomptarget/deviceRTLs/common/src/parallel.cu
@@ -227,10 +227,8 @@
 }
 
 // This routine is always called by the team master..
-EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn,
-   int16_t IsOMPRuntimeInitialized) {
+EXTERN void __kmpc_kernel_prepare_parallel(void *WorkFn) {
   PRINT0(LD_IO, "call to __kmpc_kernel_prepare_parallel\n");
-  ASSERT0(LT_FUSSY, IsOMPRuntimeInitialized, "Expected initialized runtime.");
 
   omptarget_nvptx_workFn = WorkFn;
 
@@ -275,12 +273,9 @@
 // returns True if this thread is active, else False.
 //
 // Only the worker threads call this routine.
-EXTERN bool __kmpc_kernel_parallel(void **WorkFn,
-   int16_t IsOMPRuntimeInitialized) {
+EXTERN bool __kmpc_kernel_parallel(void **WorkFn) {
   PRINT0(LD_IO | LD_PAR, "call to __kmpc_kernel_parallel\n");
 
-  ASSERT0(LT_FUSSY, IsOMPRuntimeInitialized, "Expected initialized runtime.");
-
   // Work function and arguments for L1 parallel region.
   *WorkFn = omptarget_nvptx_workFn;
 
Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -584,6 +584,11 @@
 __OMP_RTL(__kmpc_task_allow_completion_event, false, VoidPtr, IdentPtr,
   /* Int */ Int32, /* kmp_task_t */ VoidPtr)
 
+/// Note that device runtime functions (in the following) do not necessarily
+/// need attributes as we expect to see the definitions.
+__OMP_RTL(__kmpc_kernel_parallel, false, Int1, VoidPtrPtr)
+__OMP_RTL(__kmpc_kernel_prepare_parallel, false, Void, VoidPtr)
+
 __OMP_RTL(__last, false, Void, )
 
 #undef __OMP_RTL
Index: clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
===
--- clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
+++ clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp
@@ -88,7 +88,7 @@
   // CHECK: [[I_ADDR:%.+]] = getelementptr inbounds [[GLOB_TY]], [[GLOB_TY]]* [[RD]], i32 0, i32 0
   //
   // CHECK: call void @__kmpc_for_static_init_4(
-  // CHECK: call void @__kmpc_kernel_prepare_parallel(i8* bitcast (void (i16, i32)* @{{.+}} to i8*), i16 1)
+  // CHECK: call void @__kmpc_kernel_prepare_parallel(i8* bitcast (void (i16, i32)* @{{.+}} to i8*))
   // CHECK: call void @__kmpc_begin_sharing_variables(i8*** [[SHARED_VARS_PTR:%.+]], i{{64|32}} 1)
   // CHECK: [[SHARED_VARS_BUF:%.+]] = load i8**, i8*** [[SHARED_VARS_PTR]],
   // CHECK: [[VARS_BUF:%.+]] = getelementptr inbounds i8*, i8** [[SHARED_VARS_BUF]], i{{64|32}} 0
Index: clang/test/OpenMP/nvptx_target_teams_codegen.cpp
===