[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-04 Thread Pushpinder Singh via Phabricator via cfe-commits 
pdhaliwal abandoned this revision.
pdhaliwal marked an inline comment as done.
pdhaliwal added inline comments.



Comment at: clang/lib/Driver/ToolChains/HIP.cpp:389
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {

sameerds wrote:
> pdhaliwal wrote:
> > arsenm wrote:
> > > Needs a comment? I don't understand why openmp is any different here
> > `HostTC.TranslateArgs` (HostTC = Generic_GCC, Line#383) returns `DAL` which 
> >  contains `Args` when offloadkind is OFK_OpenMP (for all other cases, it 
> > returns nullptr). Thus, Line#{390,391} is just duplicating the arguments 
> > which are propagating to the opt, llc, etc. commands.
> > Ref: https://clang.llvm.org/doxygen/Gnu_8cpp_source.html#l02966
> I think @arsenm was asking for a comment in the code itself.
> 
> Also, I am not sufficiently familiar with the design here, but why is the HIP 
> driver checking for OpenMP offloading? Is the offloaded region treated as HIP 
> code? It seems a bit strange that we are mixing two different things in the 
> same driver.
@sameerds you are correct, openmp should not be mixing things in HIP driver. 
So, I am dropping this patch before it creates more confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80996



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

[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-03 Thread Sameer Sahasrabuddhe via Phabricator via cfe-commits 
sameerds added inline comments.



Comment at: clang/lib/Driver/ToolChains/HIP.cpp:389
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {

pdhaliwal wrote:
> arsenm wrote:
> > Needs a comment? I don't understand why openmp is any different here
> `HostTC.TranslateArgs` (HostTC = Generic_GCC, Line#383) returns `DAL` which  
> contains `Args` when offloadkind is OFK_OpenMP (for all other cases, it 
> returns nullptr). Thus, Line#{390,391} is just duplicating the arguments 
> which are propagating to the opt, llc, etc. commands.
> Ref: https://clang.llvm.org/doxygen/Gnu_8cpp_source.html#l02966
I think @arsenm was asking for a comment in the code itself.

Also, I am not sufficiently familiar with the design here, but why is the HIP 
driver checking for OpenMP offloading? Is the offloaded region treated as HIP 
code? It seems a bit strange that we are mixing two different things in the 
same driver.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80996



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

[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-02 Thread Pushpinder Singh via Phabricator via cfe-commits 
pdhaliwal added inline comments.



Comment at: clang/lib/Driver/ToolChains/HIP.cpp:389
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {

arsenm wrote:
> Needs a comment? I don't understand why openmp is any different here
`HostTC.TranslateArgs` (HostTC = Generic_GCC, Line#383) returns `DAL` which  
contains `Args` when offloadkind is OFK_OpenMP (for all other cases, it returns 
nullptr). Thus, Line#{390,391} is just duplicating the arguments which are 
propagating to the opt, llc, etc. commands.
Ref: https://clang.llvm.org/doxygen/Gnu_8cpp_source.html#l02966


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80996



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

[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-02 Thread Matt Arsenault via Phabricator via cfe-commits 
arsenm added inline comments.



Comment at: clang/lib/Driver/ToolChains/HIP.cpp:389
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {

Needs a comment? I don't understand why openmp is any different here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80996



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

[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-02 Thread Pushpinder Singh via Phabricator via cfe-commits 
pdhaliwal updated this revision to Diff 267870.
pdhaliwal added a comment.

Added lit test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80996

Files:
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/test/Driver/hip-openmp-arguments.c


Index: clang/test/Driver/hip-openmp-arguments.c
===
--- /dev/null
+++ clang/test/Driver/hip-openmp-arguments.c
@@ -0,0 +1,13 @@
+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// RUN: %clang -### -target x86_64-pc-linux-gnu -fopenmp \
+// RUN:   -fopenmp-targets=amdgcn-amd-amdhsa -nogpulib \
+// RUN:   -mllvm -amdgpu-dump-hsa-metadata \
+// RUN:   %s 2>&1 | FileCheck %s
+
+// CHECK: [[CLANG:".*clang.*"]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-pc-linux-gnu" "-emit-obj"
+// CHECK-SAME: {{.*}} "-fopenmp"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-dump-hsa-metadata" 
"-fopenmp-is-device" {{.*}}
Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -386,8 +386,10 @@
 
   const OptTable  = getDriver().getOpts();
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {
+  DAL->append(A);
+}
   }
 
   if (!BoundArch.empty()) {


Index: clang/test/Driver/hip-openmp-arguments.c
===
--- /dev/null
+++ clang/test/Driver/hip-openmp-arguments.c
@@ -0,0 +1,13 @@
+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+
+// RUN: %clang -### -target x86_64-pc-linux-gnu -fopenmp \
+// RUN:   -fopenmp-targets=amdgcn-amd-amdhsa -nogpulib \
+// RUN:   -mllvm -amdgpu-dump-hsa-metadata \
+// RUN:   %s 2>&1 | FileCheck %s
+
+// CHECK: [[CLANG:".*clang.*"]] "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// CHECK-SAME: "-aux-triple" "x86_64-pc-linux-gnu" "-emit-obj"
+// CHECK-SAME: {{.*}} "-fopenmp"
+// CHECK-SAME: {{.*}} "-mllvm" "-amdgpu-dump-hsa-metadata" "-fopenmp-is-device" {{.*}}
Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -386,8 +386,10 @@
 
   const OptTable  = getDriver().getOpts();
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {
+  DAL->append(A);
+}
   }
 
   if (!BoundArch.empty()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-02 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl added a comment.

Can we have a lit test? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80996



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

[PATCH] D80996: [AMDGPU][OpenMP] Fix duplicate copies of arguments in commands

2020-06-02 Thread Pushpinder Singh via Phabricator via cfe-commits 
pdhaliwal created this revision.
pdhaliwal added reviewers: yaxunl, msearles, sameerds.
Herald added subscribers: cfe-commits, sstefan1, guansong, t-tye, tpr, 
dstuttard, wdng, kzhuravl.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

When offloading kind is OFK_OpenMP, the host toolchain (Generic_GCC)
returns DerivedArgList which copies input arguments along with few
other arguments. HIPToolchain again copies input arguments to the
same DerivedArgList.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80996

Files:
  clang/lib/Driver/ToolChains/HIP.cpp


Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -386,8 +386,10 @@
 
   const OptTable  = getDriver().getOpts();
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {
+  DAL->append(A);
+}
   }
 
   if (!BoundArch.empty()) {


Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -386,8 +386,10 @@
 
   const OptTable  = getDriver().getOpts();
 
-  for (Arg *A : Args) {
-DAL->append(A);
+  if (DeviceOffloadKind != Action::OFK_OpenMP) {
+for (Arg *A : Args) {
+  DAL->append(A);
+}
   }
 
   if (!BoundArch.empty()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits