subject:"\[PATCH\] D101901\: \[AMDGPU\]\[OpenMP\] Fix clang driver crash when provided \-c"

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

2021-05-05 Thread Pushpinder Singh via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1f5cacfcb845: [AMDGPU][OpenMP] Fix clang driver crash when 
provided -c (authored by pdhaliwal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101901

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/amdgpu-openmp-toolchain.c


Index: clang/test/Driver/amdgpu-openmp-toolchain.c
===
--- clang/test/Driver/amdgpu-openmp-toolchain.c
+++ clang/test/Driver/amdgpu-openmp-toolchain.c
@@ -26,12 +26,14 @@
 // CHECK-PHASES: 6: preprocessor, {5}, cpp-output, (device-openmp)
 // CHECK-PHASES: 7: compiler, {6}, ir, (device-openmp)
 // CHECK-PHASES: 8: offload, "host-openmp (x86_64-unknown-linux-gnu)" {2}, 
"device-openmp (amdgcn-amd-amdhsa)" {7}, ir
-// CHECK-PHASES: 9: linker, {8}, image, (device-openmp)
-// CHECK-PHASES: 10: offload, "device-openmp (amdgcn-amd-amdhsa)" {9}, image
-// CHECK-PHASES: 11: clang-offload-wrapper, {10}, ir, (host-openmp)
-// CHECK-PHASES: 12: backend, {11}, assembler, (host-openmp)
-// CHECK-PHASES: 13: assembler, {12}, object, (host-openmp)
-// CHECK-PHASES: 14: linker, {4, 13}, image, (host-openmp)
+// CHECK-PHASES: 9: backend, {8}, assembler, (device-openmp)
+// CHECK-PHASES: 10: assembler, {9}, object, (device-openmp)
+// CHECK-PHASES: 11: linker, {10}, image, (device-openmp)
+// CHECK-PHASES: 12: offload, "device-openmp (amdgcn-amd-amdhsa)" {11}, image
+// CHECK-PHASES: 13: clang-offload-wrapper, {12}, ir, (host-openmp)
+// CHECK-PHASES: 14: backend, {13}, assembler, (host-openmp)
+// CHECK-PHASES: 15: assembler, {14}, object, (host-openmp)
+// CHECK-PHASES: 16: linker, {4, 15}, image, (host-openmp)
 
 // handling of --libomptarget-amdgcn-bc-path
 // RUN:   %clang -### --target=x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa 
-march=gfx803 
--libomptarget-amdgcn-bc-path=%S/Inputs/hip_dev_lib/libomptarget-amdgcn-gfx803.bc
 %s 2>&1 | FileCheck %s --check-prefix=CHECK-LIBOMPTARGET
@@ -58,3 +60,14 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SAVE-ASM
 // CHECK-SAVE-ASM: llc{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-linked.bc" 
"-mtriple=amdgcn-amd-amdhsa" "-mcpu=gfx906" "-filetype=asm" 
"-o"{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906.s"
 // CHECK-SAVE-ASM: llc{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-linked.bc" 
"-mtriple=amdgcn-amd-amdhsa" "-mcpu=gfx906" "-filetype=obj" 
"-o"{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906.o"
+
+// check the handling of -c
+// RUN:   env LIBRARY_PATH=%S/Inputs/hip_dev_lib %clang -ccc-print-bindings -c 
--target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa 
-Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 -save-temps %s 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=CHECK-C
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang",
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang",{{.*}}output: "[[HOST_BC:.*]]"
+// CHECK-C: "amdgcn-amd-amdhsa" - "clang",{{.*}}output: "[[DEVICE_I:.*]]"
+// CHECK-C: "amdgcn-amd-amdhsa" - "clang", inputs: ["[[DEVICE_I]]", 
"[[HOST_BC]]"]
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang"
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang::as"
+// CHECK-C: "x86_64-unknown-linux-gnu" - "offload bundler"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -3100,16 +3100,8 @@
   }
 
   // By default, we produce an action for each device arch.
-  for (unsigned I = 0; I < ToolChains.size(); ++I) {
-Action * = OpenMPDeviceActions[I];
-// AMDGPU does not support linking of object files, so we skip
-// assemble and backend actions to produce LLVM IR.
-if (ToolChains[I]->getTriple().isAMDGCN() &&
-(CurPhase == phases::Assemble || CurPhase == phases::Backend))
-  continue;
-
+  for (Action * : OpenMPDeviceActions)
 A = C.getDriver().ConstructPhaseAction(C, Args, CurPhase, A);
-  }
 
   return ABRT_Success;
 }
@@ -4594,6 +4586,25 @@
   if (!T)
 return InputInfo();
 
+  if (BuildingForOffloadDevice &&
+  A->getOffloadingDeviceKind() == Action::OFK_OpenMP) {
+if (TC->getTriple().isAMDGCN()) {
+  // AMDGCN treats backend and assemble actions as no-op because
+  // linker does not support object files.
+  if (const BackendJobAction *BA = dyn_cast(A)) {
+return BuildJobsForAction(C, *BA->input_begin(), TC, BoundArch,
+  AtTopLevel, MultipleArchs, LinkingOutput,
+  CachedResults, TargetDeviceOffloadKind);
+  }
+
+  if (const AssembleJobAction *AA = dyn_cast(A)) {
+return BuildJobsForAction(C, *AA->input_begin(), TC, BoundArch,
+  AtTopLevel, MultipleArchs, LinkingOutput,
+

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

2021-05-05 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added a comment.

Cool, this makes sense :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101901

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

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

2021-05-05 Thread Jon Chesterfield via Phabricator via cfe-commits

JonChesterfield added a comment.

@jdoerfert This drops the logic for save-temps that I remember being 
contentious.

Doing a no-op backend pass instead of skipping over it is probably more robust.




Comment at: clang/lib/Driver/Driver.cpp:4600
+
+  if (const AssembleJobAction *AA = dyn_cast(A)) {
+return BuildJobsForAction(C, *AA->input_begin(), TC, BoundArch,

I wonder if this would be clearer as if (isa<>() || isa<>()) followed by a 
ternary to pick the second argument to BuildJobs. Overall I think I prefer the 
separate calls, as currently written.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101901

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

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

2021-05-05 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal created this revision.
pdhaliwal added reviewers: JonChesterfield, ronlieb, jdoerfert, ye-luo, 
tianshilei1992.
Herald added subscribers: kerbowa, guansong, t-tye, tpr, dstuttard, yaxunl, 
nhaehnle, jvesely, kzhuravl.
pdhaliwal requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1, wdng.
Herald added a project: clang.

The offload action is used in four different ways as explained
in Driver.cpp:4495. When -c is present, the final phase will be
assemble (linker when -c is not present). However, this phase
is skipped according to D96769  for amdgcn. 
So, offload action
arrives into following situation,

compile (device) ---> offload ---> offload

without -c the chain looks like,
 compile (device) ---> offload ---> linker (device)

---> offload

The former situation creates an unhandled case which causes
problem. The solution presented in this patch delays the D96769 

logic until job creation time. This keeps the offload action
in the 1 of the 4 specified situations.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101901

Files:
  clang/lib/Driver/Driver.cpp
  clang/test/Driver/amdgpu-openmp-toolchain.c


Index: clang/test/Driver/amdgpu-openmp-toolchain.c
===
--- clang/test/Driver/amdgpu-openmp-toolchain.c
+++ clang/test/Driver/amdgpu-openmp-toolchain.c
@@ -26,12 +26,14 @@
 // CHECK-PHASES: 6: preprocessor, {5}, cpp-output, (device-openmp)
 // CHECK-PHASES: 7: compiler, {6}, ir, (device-openmp)
 // CHECK-PHASES: 8: offload, "host-openmp (x86_64-unknown-linux-gnu)" {2}, 
"device-openmp (amdgcn-amd-amdhsa)" {7}, ir
-// CHECK-PHASES: 9: linker, {8}, image, (device-openmp)
-// CHECK-PHASES: 10: offload, "device-openmp (amdgcn-amd-amdhsa)" {9}, image
-// CHECK-PHASES: 11: clang-offload-wrapper, {10}, ir, (host-openmp)
-// CHECK-PHASES: 12: backend, {11}, assembler, (host-openmp)
-// CHECK-PHASES: 13: assembler, {12}, object, (host-openmp)
-// CHECK-PHASES: 14: linker, {4, 13}, image, (host-openmp)
+// CHECK-PHASES: 9: backend, {8}, assembler, (device-openmp)
+// CHECK-PHASES: 10: assembler, {9}, object, (device-openmp)
+// CHECK-PHASES: 11: linker, {10}, image, (device-openmp)
+// CHECK-PHASES: 12: offload, "device-openmp (amdgcn-amd-amdhsa)" {11}, image
+// CHECK-PHASES: 13: clang-offload-wrapper, {12}, ir, (host-openmp)
+// CHECK-PHASES: 14: backend, {13}, assembler, (host-openmp)
+// CHECK-PHASES: 15: assembler, {14}, object, (host-openmp)
+// CHECK-PHASES: 16: linker, {4, 15}, image, (host-openmp)
 
 // handling of --libomptarget-amdgcn-bc-path
 // RUN:   %clang -### --target=x86_64-unknown-linux-gnu -fopenmp 
-fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa 
-march=gfx803 
--libomptarget-amdgcn-bc-path=%S/Inputs/hip_dev_lib/libomptarget-amdgcn-gfx803.bc
 %s 2>&1 | FileCheck %s --check-prefix=CHECK-LIBOMPTARGET
@@ -58,3 +60,14 @@
 // RUN:   | FileCheck %s --check-prefix=CHECK-SAVE-ASM
 // CHECK-SAVE-ASM: llc{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-linked.bc" 
"-mtriple=amdgcn-amd-amdhsa" "-mcpu=gfx906" "-filetype=asm" 
"-o"{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906.s"
 // CHECK-SAVE-ASM: llc{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906-linked.bc" 
"-mtriple=amdgcn-amd-amdhsa" "-mcpu=gfx906" "-filetype=obj" 
"-o"{{.*}}amdgpu-openmp-toolchain-{{.*}}-gfx906.o"
+
+// check the handling of -c
+// RUN:   env LIBRARY_PATH=%S/Inputs/hip_dev_lib %clang -ccc-print-bindings -c 
--target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa 
-Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 -save-temps %s 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=CHECK-C
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang",
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang",{{.*}}output: "[[HOST_BC:.*]]"
+// CHECK-C: "amdgcn-amd-amdhsa" - "clang",{{.*}}output: "[[DEVICE_I:.*]]"
+// CHECK-C: "amdgcn-amd-amdhsa" - "clang", inputs: ["[[DEVICE_I]]", 
"[[HOST_BC]]"]
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang"
+// CHECK-C: "x86_64-unknown-linux-gnu" - "clang::as"
+// CHECK-C: "x86_64-unknown-linux-gnu" - "offload bundler"
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -3100,16 +3100,8 @@
   }
 
   // By default, we produce an action for each device arch.
-  for (unsigned I = 0; I < ToolChains.size(); ++I) {
-Action * = OpenMPDeviceActions[I];
-// AMDGPU does not support linking of object files, so we skip
-// assemble and backend actions to produce LLVM IR.
-if (ToolChains[I]->getTriple().isAMDGCN() &&
-(CurPhase == phases::Assemble || CurPhase == phases::Backend))
-  continue;
-
+  for (Action * : OpenMPDeviceActions)
 A = C.getDriver().ConstructPhaseAction(C, Args, CurPhase, A);
-  }

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

[PATCH] D101901: [AMDGPU][OpenMP] Fix clang driver crash when provided -c

4 matches

Site Navigation

Mail list logo

Footer information