[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Samuel Antao via Phabricator via cfe-commits]

sfantao added a comment.

@gtbercea, thanks for the patch.

> INTEROPERABILITY WITH OTHER COMPILERS: These bundled object files can end up 
> being passed between Clang and other compilers which may lead to 
> incompatibilities: passing a bundled file from Clang to another compiler 
> would lead to that compiler not being able to unbundle it. Passing an 
> unbundled object file to Clang and therefore Clang not knowing that it 
> doesn't need to unbundle it.

I don't understand the interoperability issue here. Clang convention to create 
the offloading records in the host binary is specific to clang and was 
discussed, defined and documented. We do not expect other compilers to 
understand it as we don't expect clang to understand that from other compilers. 
In terms of the binary itself, it is an ELF file that can be understood by 
other compilers/tools - albeit the linking would probably fail if the device 
section addresses are not defined.

I think that what you mean here is that by forcing nvcc as the host linker when 
one of the targets is a NVPTX target, you shift the (un)bundling part to nvlink 
and you will be compatible with host compilers supported by nvcc.

I understand that at the moment there is no immediate need for combining 
multiple targets, and there is an immediate need to support archives with 
offloading code. Therefore, changing the host linker in function of the offload 
target seems reasonable as that is what would help most users.

I just want to note that the separation of the toolchains in the driver and the 
support for multiple toolchains in the same binary were part of the reason we 
converged to the current design. The idea was to have a generic driver with all 
the details relative to bundling formats handled by a separate tool, the 
bundler. Of course the requirements can be reviewed at any time and priorities 
can change. However, I think it would be cleaner to have the nvlink compatible 
binary generated by the bundler in the current design. Just my two cents.




Comment at: include/clang/Driver/Compilation.h:126
+  /// Whether the clang-offload-bundler can be skipped.
+  bool skipOffloadBundler = false;
+

Use CamelCase for class local variables.



Comment at: include/clang/Driver/Compilation.h:312
+  /// \param skipBundler - bool value set once by the driver.
+  void setSkipOffloadBundler(bool skipBundler);
+

Why is this a property of the compilation and not of a set of actions referring 
to a given target? That would allow one to combine in the same compilation 
targets requiring the bundler and targets that wouldn't. 



Comment at: lib/Driver/Compilation.cpp:276
+void Compilation::setSkipOffloadBundler(bool skipBundler) {
+  skipOffloadBundler = skipBundler;
+}

Given the logic you have below, you are assuming this is not set to false ever. 
It would be wise to get an assertion here in case you end up having toolchains 
skipping and others don't. If that is just not supported a diagnostic should be 
added instead.

The convention is that local variables use CamelCase.



Comment at: lib/Driver/Driver.cpp:2927
+if (OpenMPTargets->getValues().size() > 0) {
+  unsigned triplesRequiringBundler = 0;
+  for (const char *Val : OpenMPTargets->getValues()) {

CamelCase



Comment at: lib/Driver/Driver.cpp:2943
+}
+  }
+

Can you just implement this check in the definition of `Compilation: 
canSkipClangOffloadBundler` and get rid of `setSkipOffloadBundler`? All the 
requirted information is already in `Compilation` under `C.getInputArgs()`.



Comment at: lib/Driver/Driver.cpp:2962
+// is ld instead but this needs to be replaced.
+canDoPartialLinking = LinkerName.endswith("/ld");
+  }

In the current implementation there is no distinction between what is meant for 
Windows/Linux. This check would only work on Linux and the test below would 
fail for bots running windows.

Also, I think it makes more sense to have this check part of the `Toolchain` 
instead of doing it in the driver. The `Toolchain` definition knows the names 
of the third-party executables, the driver doesn't. 



Comment at: lib/Driver/ToolChains/Clang.cpp:5504
+  StringRef LinkerName = getToolChain().GetLinkerPath();
+  assert(LinkerName.endswith("/ld") && "Partial linking not supported.");
+

I believe this check should be done when the toolchain is created with all the 
required diagnostics. What happens if the linker does not support partial 
linking?



Comment at: lib/Driver/ToolChains/Cuda.cpp:536
+  }
 }
 

What prevents all this from being done in the bundler? If I understand it 
correctly, if the bundler implements this wrapping all the checks for 
librariers wouldn't be required and, only two changes would be required in

[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Samuel Antao via Phabricator via cfe-commits]

sfantao added a comment.

Just to clarify one thing in my last comment:

When I say that we didn't aim at having clang compatible with other compilers, 
I mean the OpenMP offloading descriptors, where all the variables and 
offloading entry points are. Of course we want to allow the resulting binaries 
to be compatible with linkers taking inputs of other compilers, so that you can 
have, e.g., OpenMP and CUDA supported in the same executable, even though 
working independently.


Repository:
  rC Clang

https://reviews.llvm.org/D47394



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

[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Samuel Antao via Phabricator via cfe-commits]

sfantao added a comment.

Hi Doru,

Thanks for updating the patch. I've a few comments below.




Comment at: include/clang/Driver/Compilation.h:312
+  /// \param skipBundler - bool value set once by the driver.
+  void setSkipOffloadBundler(bool skipBundler);
+

gtbercea wrote:
> sfantao wrote:
> > Why is this a property of the compilation and not of a set of actions 
> > referring to a given target? That would allow one to combine in the same 
> > compilation targets requiring the bundler and targets that wouldn't. 
> This was a way to pass this information to the OpenMP NVPTX device toolchain.
> 
> Both the Driver OpenMP NVPTX toolchain need to agree on the usage of the new 
> scheme (proposed in this patch) or the old scheme (the one that is in the 
> compiler today).
> 
> 
I understand, but the way I see it is that it is the toolchain that skips the 
bundler not the compilation. I understand that as of this patch, you skip only 
if there is a single nvptx target. If you have more than one target, as some 
tests do, some toolchains will still need the bundler. So, we are making what 
happens with the nvptx target dependent of other toolchains. Is this an 
intended effect of this patch?



Comment at: lib/Driver/Compilation.cpp:276
+void Compilation::setSkipOffloadBundler(bool skipBundler) {
+  skipOffloadBundler = skipBundler;
+}

gtbercea wrote:
> sfantao wrote:
> > Given the logic you have below, you are assuming this is not set to false 
> > ever. It would be wise to get an assertion here in case you end up having 
> > toolchains skipping and others don't. If that is just not supported a 
> > diagnostic should be added instead.
> > 
> > The convention is that local variables use CamelCase.
> The checks I added in the Driver will set this flag to true if all toolchains 
> Clang offloads to support the skipping of the bundler/unbundler for object 
> files. Currently only NVPTX toolchain can skip the bundler/unbundler for 
> object files so the code path in this patch will be taken only for:
> 
> -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda
Ok, if that is the case, just add an assertion here.



Comment at: lib/Driver/Driver.cpp:2943
+}
+  }
+

gtbercea wrote:
> sfantao wrote:
> > Can you just implement this check in the definition of `Compilation: 
> > canSkipClangOffloadBundler` and get rid of `setSkipOffloadBundler`? All the 
> > requirted information is already in `Compilation` under `C.getInputArgs()`.
> The driver needs to have the result of this check available. The flag is 
> passed to the step which adds host-device dependencies. If the bundler can be 
> skipped then the unbundling action is not required.
> 
> I guess this could be implemented in Compilation. Even so I would like it to 
> happen only once like it does here and not every time someone queries the 
> "can I skip the bundler" flag.
> 
> I wanted this check to happen only once hence why I put in on the driver 
> side. The result of this check needs to be available in Driver.cpp and in 
> Cuda.cpp files (see usage in this patch). Compilation keeps track of the flag 
> because skipping the bundler is an all or nothing flag: you can skip the 
> bundler/unbundler for object files if and only if all toolchains you are 
> offloading to can skip it.
> 
Right, in these circumstances "can skip bundler" is the same as "do I have a 
single toolchain" and "is that toolchain nvptx". This is fairly inexpensive to 
do, so I don't really see the need to record this state in the driver. It will 
also be clearer what are the conditions for which you skip the bundler.



Comment at: lib/Driver/ToolChains/Cuda.cpp:496
+  ? CudaVirtualArchToString(VirtualArchForCudaArch(gpu_arch))
+  : GPUArch.str().c_str();
+  const char *PtxF =

Why don't create fatbins instead of cubins in all cases. For the purposes of 
OpenMP they are equivalent, i.e. nvlink can interpret them in the same way, no?



Comment at: lib/Driver/ToolChains/Cuda.cpp:517
+//   --image=profile=sm_35@compute_35,file=ompprint.compute_35.sm_35.cubin
+//   --embedded-fatbin=ompprint.fatbin.c --cuda --device-c
+const char *Exec = Args.MakeArgString(TC.GetProgramPath("fatbinary"));

I'd move this comment to the top of this session so that we know what is going 
on in the code above.



Comment at: lib/Driver/ToolChains/Cuda.cpp:524
+// the hash of the full path of the file.
+std::hash hash_fn;
+size_t hash = hash_fn(llvm::sys::path::filename(Output.getFilename()));

CamelCase



Comment at: lib/Driver/ToolChains/Cuda.cpp:664
+  // Anything that's not a file name is potentially a static library
+  // so treat it as such.
+  if (C.canSkipOffloadBundler())

So, what if it is not a static library?

[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Samuel Antao via Phabricator via cfe-commits]

sfantao added inline comments.



Comment at: include/clang/Driver/Compilation.h:312
+  /// \param skipBundler - bool value set once by the driver.
+  void setSkipOffloadBundler(bool skipBundler);
+

gtbercea wrote:
> sfantao wrote:
> > gtbercea wrote:
> > > sfantao wrote:
> > > > Why is this a property of the compilation and not of a set of actions 
> > > > referring to a given target? That would allow one to combine in the 
> > > > same compilation targets requiring the bundler and targets that 
> > > > wouldn't. 
> > > This was a way to pass this information to the OpenMP NVPTX device 
> > > toolchain.
> > > 
> > > Both the Driver OpenMP NVPTX toolchain need to agree on the usage of the 
> > > new scheme (proposed in this patch) or the old scheme (the one that is in 
> > > the compiler today).
> > > 
> > > 
> > I understand, but the way I see it is that it is the toolchain that skips 
> > the bundler not the compilation. I understand that as of this patch, you 
> > skip only if there is a single nvptx target. If you have more than one 
> > target, as some tests do, some toolchains will still need the bundler. So, 
> > we are making what happens with the nvptx target dependent of other 
> > toolchains. Is this an intended effect of this patch?
> Bundler is skipped only for the OpenMP NVPTX toolchain. I'm not sure what you 
> mean by "other toolchain".
Is skipped for the NVPTX toolchain if there are no "other" device toolchains 
requested. Say I have a working pipeline that does static linking with nvptx 
correctly. Then on top of that I add another device to `-fopenmp-targets`, that 
pipeline will now fail even for nvptx, right?



Comment at: lib/Driver/Compilation.cpp:276
+void Compilation::setSkipOffloadBundler(bool skipBundler) {
+  skipOffloadBundler = skipBundler;
+}

gtbercea wrote:
> sfantao wrote:
> > gtbercea wrote:
> > > sfantao wrote:
> > > > Given the logic you have below, you are assuming this is not set to 
> > > > false ever. It would be wise to get an assertion here in case you end 
> > > > up having toolchains skipping and others don't. If that is just not 
> > > > supported a diagnostic should be added instead.
> > > > 
> > > > The convention is that local variables use CamelCase.
> > > The checks I added in the Driver will set this flag to true if all 
> > > toolchains Clang offloads to support the skipping of the 
> > > bundler/unbundler for object files. Currently only NVPTX toolchain can 
> > > skip the bundler/unbundler for object files so the code path in this 
> > > patch will be taken only for:
> > > 
> > > -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda
> > Ok, if that is the case, just add an assertion here.
> If one of the toolchains in the list of toolchains can't skip then none of 
> them skip. If all can skip then they all skip. What assertion would you like 
> me to add?
If SkipOffloadBundler is set to true you don't expect it to be set to false 
afterwards, right? That should be asserted.



Comment at: lib/Driver/ToolChains/Cuda.cpp:496
+  ? CudaVirtualArchToString(VirtualArchForCudaArch(gpu_arch))
+  : GPUArch.str().c_str();
+  const char *PtxF =

gtbercea wrote:
> sfantao wrote:
> > Why don't create fatbins instead of cubins in all cases. For the purposes 
> > of OpenMP they are equivalent, i.e. nvlink can interpret them in the same 
> > way, no?
> I'm not sure why the comment is attached to this particular line in the code.
> 
> But the reason why I don't use fatbins everywhere is because I want to leave 
> the previous toolchain intact. So when the bundler is not skipped we do 
> precisely what we did before.
The comment was here, because this is where you generate the command to create 
the fatbin - no other intended meaning.

Given that fatbin can be linked with nvlink to get a device cubin the toolchain 
won't need to change regardless of whether bundling is used or not, for the 
bundler the device images are just bits. 


Repository:
  rC Clang

https://reviews.llvm.org/D47394



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

[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Samuel Antao via Phabricator via cfe-commits]

sfantao added a comment.

> In a discussion off-list I proposed adding constructor functions to all 
> object files and handle them like shared libraries are already handled today 
> (ie register separately and let the runtime figure out how to relocate 
> symbols in different translation units). I don't have an implementation of 
> that approach so I can't claim that it works and doesn't have a huge 
> performance impact (which we don't want either), but it should be agnostic of 
> the offloading target so it may be worth investigating.

I don't understand how this would work. Doing something like that would require 
reimplementing the GPU-code linker, which requires knowing proprietary 
information of the GPU binary format. I would know how to resolve all the 
relocations in the device code. In my view, the solution would only work (or at 
least be more easily implemented) if we don't have relocatable code.

> Assuming we do proceed with back-to-CUDA approach, one thing I'd consider 
> would be using clang's -fcuda-include-gpubinary option which CUDA uses to 
> include GPU code into the host object. You may be able to use it to avoid 
> compiling and partially linking .fatbin and host .o.

Cool, I agree this is worth investigating.




Comment at: lib/Driver/ToolChains/Cuda.cpp:536
+  }
 }
 

gtbercea wrote:
> sfantao wrote:
> > What prevents all this from being done in the bundler? If I understand it 
> > correctly, if the bundler implements this wrapping all the checks for 
> > librariers wouldn't be required and, only two changes would be required in 
> > the driver:
> > 
> > - generate fatbin instead of cubin. This is straightforward to do by 
> > changing the device assembling job. In terms of the loading of the kernels 
> > by the device API, doing it through fatbin or cubin should be equivalent 
> > except that fatbin enables storing the PTX format and JIT for newer GPUs.
> > - Use NVIDIA linker as host linker.
> > 
> > This last requirement could be problematic if we get two targets attempting 
> >  to use different (incompatible linkers). If we get this kind of 
> > incompatibility we should get the appropriate diagnostic.
> What prevents it is the fact that the bundler is called AFTER the HOST and 
> DEVICE object files have been produced. The creation of the fatbin (FATBINARY 
> + CALNG++) needs to happen within the NVPTX toolchain.
> 
Why does it have to happen in NVPTX toolchain, you are making the NVPTX 
toolchain generate an ELF object from another toolchain, right? What I'm 
suggesting is to do the stuff that mixes two (or more) toolchains in the 
bundler. Your inputs are still a fatbin and a host file.   



Comment at: test/Driver/openmp-offload.c:497
 // RUN:   %clang -###  -fopenmp=libomp -o %t.out -lsomelib -target 
powerpc64le-linux 
-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu %t.i 
-no-canonical-prefixes 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-UBJOBS %s
 // RUN:   %clang -### -fopenmp=libomp -o %t.out -lsomelib -target 
powerpc64le-linux 
-fopenmp-targets=powerpc64le-ibm-linux-gnu,x86_64-pc-linux-gnu %t.i -save-temps 
-no-canonical-prefixes 2>&1 \

gtbercea wrote:
> gtbercea wrote:
> > sfantao wrote:
> > > We need a test for the static linking. The host linker has to be nvcc in 
> > > that case, right?
> > The host linker is "ld". The "bundling" step is replaced (in the case of 
> > OpenMP NVPTX device offloading only) by a call to "ld -r" to partially link 
> > the 2 object files: the object file produced by the HOST toolchain and the 
> > object file produced by the OpenMP NVPTX device offloading toolchain 
> > (because we want to produce a single output).
> nvcc is not called at all in this patch.
Ok, so how do you link device code? I.e. if you have two compilation units that 
depend on each other (some definition in one unit is used in the other), where 
are they linked together? Something has to understand the two files resulting 
from your "ld -r" step, my understanding is that that something is nvcc that 
calls nvlink behind the scenes, right? So, nvcc will do the unbundling+linking 
bit, right?


Repository:
  rC Clang

https://reviews.llvm.org/D47394



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

[PATCH] D21856: [Driver][OpenMP] Add support to create jobs for bundling actions.

[Samuel Antao via Phabricator via cfe-commits]

sfantao added a comment.

In D21856#1338687 , @thakis wrote:

> Sorry about the years-later question, but what's the motivation for shelling 
> out to an external command here? In general, LLVM tries to use a 
> library-based approach, and LLVM went e.g. through great lengths do use an 
> integrated assembler so clang doesn't have to shell out to one. Concretely, 
> why isn't there a clang/lib/OffloadBundle library that clang and 
> clang-offload-bundler both use? Does the bundling have to happen 
> out-of-process? And if so, why isn't the process something like `clang 
> -cc1bundle` instead of a separate executable? It seems weird to make clang 
> depend on another executable next to it.
>
> I apologize for the clueless question.


@thakis, I don't think there is anything preventing the bundler from being 
driven by clang and implemented in a library. The discussions about the bundler 
were mostly around the bundling formats - so where it should be implemented was 
not as thoroughly discussed as the the formats we ended up adopting. 
Notwithstanding, there were a few reasons that motivated the current design:

- The bundler should have a driver: users/developers should be able to easily 
pack/unpack the pieces referring to different devices. It could be a `clang 
-cc1bundle` though.
- Offloading with many targets and different host/target combinations would 
require a myriad of formats that need to be used together, so they were some 
informal discussions where people seemed to prefer to separate this complexity 
from clang itself. The bundler would need some dependencies to handle object 
files that were not needed by clang (at least at the time).
- The main use of the bundler is the bundling/unbundling of object files which 
were produced by third party tools (e.g. NVIDIA tools), so there was not a 
clear advantage in integrating the bundler as a library, as one needs to spawn 
a separate process to create its dependences anyway.




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

https://reviews.llvm.org/D21856



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

[PATCH] D90436: [Bundler] Use argv[0] as the default choice for the Executable name.

[Samuel Antao via Phabricator via cfe-commits]

sfantao added a comment.

@tra, thank you for the patch. This makes sense.

The patch looks good but it has to be someone else to okay it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90436

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

[PATCH] D28298: [OpenMP] Add fields for flags in the offload entry descriptor.

[Samuel Antao via Phabricator via cfe-commits]

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

Hi Jonas,

Thanks for the review.




Comment at: lib/CodeGen/CGOpenMPRuntime.h:250-252
+  // \brief Flags associated the device global.
+  int32_t Flags;
+

Hahnfeld wrote:
> Is that intentionally not in the `protected` section below?
Yes, we have a public getter/setter to interact with that, so no need to make 
it protected.  


https://reviews.llvm.org/D28298



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

[PATCH] D28298: [OpenMP] Add fields for flags in the offload entry descriptor.

[Samuel Antao via Phabricator via cfe-commits]

sfantao updated this revision to Diff 83236.
sfantao marked 2 inline comments as done.
sfantao added a comment.

- Privatize Order, Flags and Kind of the offload entry.


https://reviews.llvm.org/D28298

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  lib/CodeGen/CGOpenMPRuntimeNVPTX.h
  test/OpenMP/target_codegen.cpp
  test/OpenMP/target_codegen_registration.cpp

Index: test/OpenMP/target_codegen_registration.cpp
===
--- test/OpenMP/target_codegen_registration.cpp
+++ test/OpenMP/target_codegen_registration.cpp
@@ -30,11 +30,11 @@
 // CHECK-DAG: [[SE:%.+]] = type { [64 x i32] }
 // CHECK-DAG: [[ST1:%.+]] = type { [228 x i32] }
 // CHECK-DAG: [[ST2:%.+]] = type { [1128 x i32] }
-// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]] }
+// CHECK-DAG: [[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]], i32, i32 }
 // CHECK-DAG: [[DEVTY:%.+]] = type { i8*, i8*, [[ENTTY]]*, [[ENTTY]]* }
 // CHECK-DAG: [[DSCTY:%.+]] = type { i32, [[DEVTY]]*, [[ENTTY]]*, [[ENTTY]]* }
 
-// TCHECK:[[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]] }
+// TCHECK:[[ENTTY:%.+]] = type { i8*, i8*, i[[SZ:32|64]], i32, i32 }
 
 // CHECK-DAG: [[A1:@.+]] = internal global [[SA]]
 // CHECK-DAG: [[A2:@.+]] = global [[SA]]
@@ -100,54 +100,54 @@
 // CHECK-NTARGET-NOT: private unnamed_addr constant [1 x i
 
 // CHECK-DAG: [[NAMEPTR1:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME1:__omp_offloading_[0-9a-f]+_[0-9a-f]+__Z.+_l[0-9]+]]\00"
-// CHECK-DAG: [[ENTRY1:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR1]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY1:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR1]], i32 0, i32 0), i[[SZ]] 0, i32 0, i32 0 }, section ".omp_offloading.entries", align 1
 // CHECK-DAG: [[NAMEPTR2:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME2:.+]]\00"
-// CHECK-DAG: [[ENTRY2:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR2]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY2:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR2]], i32 0, i32 0), i[[SZ]] 0, i32 0, i32 0 }, section ".omp_offloading.entries", align 1
 // CHECK-DAG: [[NAMEPTR3:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME3:.+]]\00"
-// CHECK-DAG: [[ENTRY3:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR3]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY3:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR3]], i32 0, i32 0), i[[SZ]] 0, i32 0, i32 0 }, section ".omp_offloading.entries", align 1
 // CHECK-DAG: [[NAMEPTR4:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME4:.+]]\00"
-// CHECK-DAG: [[ENTRY4:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR4]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY4:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR4]], i32 0, i32 0), i[[SZ]] 0, i32 0, i32 0 }, section ".omp_offloading.entries", align 1
 // CHECK-DAG: [[NAMEPTR5:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME5:.+]]\00"
-// CHECK-DAG: [[ENTRY5:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR5]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY5:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR5]], i32 0, i32 0), i[[SZ]] 0, i32 0, i32 0 }, section ".omp_offloading.entries", align 1
 // CHECK-DAG: [[NAMEPTR6:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME6:.+]]\00"
-// CHECK-DAG: [[ENTRY6:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR6]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY6:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR6]], i32 0, i32 0), i[[SZ]] 0, i32 0, i32 0 }, section ".omp_offloading.entries", align 1
 // CHECK-DAG: [[NAMEPTR7:@.+]] = internal unnamed_addr constant [{{.*}} x i8] c"[[NAME7:.+]]\00"
-// CHECK-DAG: [[ENTRY7:@.+]] = constant [[ENTTY]] { i8* @{{.*}}, i8* getelementptr inbounds ([{{.*}} x i8], [{{.*}} x i8]* [[NAMEPTR7]], i32 0, i32 0), i[[SZ]] 0 }, section ".omp_offloading.entries", align 1
+// CHECK-DAG: [[ENTRY7:@.+]] = consta

[PATCH] D28298: [OpenMP] Add fields for flags in the offload entry descriptor.

[Samuel Antao via Phabricator via cfe-commits]

sfantao added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.h:250-252
+  // \brief Flags associated the device global.
+  int32_t Flags;
+

Hahnfeld wrote:
> sfantao wrote:
> > Hahnfeld wrote:
> > > Is that intentionally not in the `protected` section below?
> > Yes, we have a public getter/setter to interact with that, so no need to 
> > make it protected.  
> Hm, `Order` and `Kind` also have public getters and are currently never set 
> from outside - will that be required in the future?
Good point, I guess those fields were being directly accessed by the child 
classes but now they are set through the constructor. I am privatizing the all 
three fields in the last diff as they don't need to be protected anymore.


https://reviews.llvm.org/D28298



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