[PATCH] D94961: [OpenMP] Add OpenMP offloading toolchain for AMDGPU

[Pushpinder Singh via Phabricator via cfe-commits]

pdhaliwal marked 3 inline comments as done.
pdhaliwal added a comment.

After addressing the review comments, I have internally verified changes on few simple test programs. They seem to be working fine.



================
Comment at: clang/lib/Driver/Driver.cpp:3004
+        }
+      }
+      // amdgcn does not support linking of object files, therefore we skip
----------------
jdoerfert wrote:
> This does not look right. IsAMDGCN is not a question that should be answered by a `-Xopenmp-target` flag. 
> 
> In general, why skip these passes here. Isn't what you really want to do, pass `-emit-llvm[-bc]` to the device compilation job?
I have removed the logic from here instead now I have added -emit-llvm-bc using addClangTargetOptions in AMDGPUOpenMP.


================
Comment at: clang/test/Driver/amdgpu-openmp-toolchain.c:2
+// REQUIRES: amdgpu-registered-target
+// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 %s 2>&1 \
+// RUN:   | FileCheck %s
----------------
jdoerfert wrote:
> Would this at all work without the march flag?
This would not work without -march flag as amdgcn is not forward/backward compatible and there is currently no way to detect system gpu arch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94961