[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-13 Thread Johannes Doerfert via Phabricator via cfe-commits 
jdoerfert added a comment.

First, as noted, I asked @jhuber6 to update this.

In D135614#3847775 , @tra wrote:

> Is that really something we need/want to do? I've never seen anyone 
> complaining about this particular issue.

I have, and not only once. Hence the request for a change.

> clang/gcc are case-sensitive for similar options, like `-march`: 
> https://godbolt.org/z/3jKzTda7h
> I think offloading options should behave consistently in that regard. If we 
> do want to make arch/CPU names case-agnostic, it should be done for all such 
> options.

I don't disagree. Major difference is though that march returns a nice list of 
options in your example.
For offload-arch the situations is less user friendly: 
https://godbolt.org/z/oWYvvjTTq

In D135614#3849656 , @b-sumner wrote:

> Also, we may want to use uppercase for other purposes in the future.

gfx90a != gfx90A but both valid? Please, no.

In D135614#3849635 , @yaxunl wrote:

> I am not sure whether it is a good idea to allow gfx90A in `--offload-arch`, 
> since it is not documented in LLVM AMDGPU usage. @b-sumner @arsenm

I'm not sure which documentation you mean but if it's 
https://llvm.org/docs/AMDGPUUsage.html, then I doubt any "end user" will have 
read it.
Arguably it's for compiler writers and not even in the frontend documentation. 
Maybe there is something else I don't know about?

In D135614#3849639 , @arsenm wrote:

> I don't really have an opinion here. I'd probably lean towards a "did you 
> mean" kind of warning

Since listing all options might not be great, I agree that we could emit a 
warning for upper/lower case situations instead. (FWIW, not only for amdgpu 
btw.)
@tra, would that be OK for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-11 Thread Brian Sumner via Phabricator via cfe-commits 
b-sumner added a comment.

Also, we may want to use uppercase for other purposes in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-11 Thread Brian Sumner via Phabricator via cfe-commits 
b-sumner added a comment.

I don't particularly see a need for this.  I am not opposed to a "did you mean" 
in the error diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-11 Thread Matt Arsenault via Phabricator via cfe-commits 
arsenm added a comment.

I don't really have an opinion here. I'd probably lean towards a "did you mean" 
kind of warning


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-11 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl added a comment.

I am not sure whether it is a good idea to allow gfx90A in `--offload-arch`, 
since it is not documented in LLVM AMDGPU usage. @b-sumner @arsenm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-10 Thread Joseph Huber via Phabricator via cfe-commits 
jhuber6 added a comment.

In D135614#3847775 , @tra wrote:

> Is that really something we need/want to do? I've never seen anyone 
> complaining about this particular issue.
>
> clang/gcc are case-sensitive for similar options, like `-march`: 
> https://godbolt.org/z/3jKzTda7h
> I think offloading options should behave consistently in that regard. If we 
> do want to make arch/CPU names case-agnostic, it should be done for all such 
> options.

This came up with someone @jdoerfert worked with and I figured it was easy to 
implement. There is a precedent for `-march` to be case sensitive, I'm not sure 
if it's an issue here since it's a little arbitrary. Alternatively we could 
make it a warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-10 Thread Artem Belevich via Phabricator via cfe-commits 
tra added a comment.

Is that really something we need/want to do? I've never seen anyone complaining 
about this particular issue.

clang/gcc are case-sensitive for similar options, like `-march`: 
https://godbolt.org/z/3jKzTda7h
I think offloading options should behave consistently in that regard. If we do 
want to make arch/CPU names case-agnostic, it should be done for all such 
options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135614

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

[PATCH] D135614: [OpenMP][CUDA][AMDGPU] Accept case insensitive subarchitecture names

2022-10-10 Thread Joseph Huber via Phabricator via cfe-commits 
jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, JonChesterfield, yaxunl, tra.
Herald added subscribers: kosarev, mattd, guansong, hiraditya, t-tye, tpr, 
dstuttard, kzhuravl.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, wdng.
Herald added projects: clang, LLVM.

The offloading toolchains are typically built by specifing a
subarchitecture during compilation via `--offload-arch=`. This patch
allows users to input subarchitecture names that ignore the case. So if
the user inputs `gfx90A` it will treat it as `gfx90a`. This is primarily
for user convenicence.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135614

Files:
  clang/lib/Basic/Cuda.cpp
  clang/test/Driver/openmp-offload-infer.c
  llvm/lib/Support/TargetParser.cpp


Index: llvm/lib/Support/TargetParser.cpp
===
--- llvm/lib/Support/TargetParser.cpp
+++ llvm/lib/Support/TargetParser.cpp
@@ -151,7 +151,7 @@
 
 AMDGPU::GPUKind llvm::AMDGPU::parseArchAMDGCN(StringRef CPU) {
   for (const auto  : AMDGCNGPUs) {
-if (CPU == C.Name)
+if (CPU.equals_insensitive(C.Name))
   return C.Kind;
   }
 
Index: clang/test/Driver/openmp-offload-infer.c
===
--- clang/test/Driver/openmp-offload-infer.c
+++ clang/test/Driver/openmp-offload-infer.c
@@ -18,6 +18,9 @@
 // RUN:   %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings 
-fopenmp \
 // RUN: --offload-arch=sm_70 --offload-arch=gfx908:sramecc+:xnack- \
 // RUN: -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-NVIDIA-AMDGPU
+// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings 
-fopenmp \
+// RUN: --offload-arch=SM_70 --offload-arch=GFX908:sramecc+:xnack- \
+// RUN: -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-NVIDIA-AMDGPU
 
 // CHECK-NVIDIA-AMDGPU: "x86_64-unknown-linux-gnu" - "clang", inputs: 
["[[INPUT:.+]]"], output: "[[HOST_BC:.+]]"
 // CHECK-NVIDIA-AMDGPU: "amdgcn-amd-amdhsa" - "clang", inputs: ["[[INPUT]]", 
"[[HOST_BC]]"], output: "[[AMD_BC:.+]]"
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -155,9 +155,10 @@
 }
 
 CudaArch StringToCudaArch(llvm::StringRef S) {
-  auto result = std::find_if(
-  std::begin(arch_names), std::end(arch_names),
-  [S](const CudaArchToStringMap ) { return S == map.arch_name; });
+  auto result = std::find_if(std::begin(arch_names), std::end(arch_names),
+ [S](const CudaArchToStringMap ) {
+   return S.equals_insensitive(map.arch_name);
+ });
   if (result == std::end(arch_names))
 return CudaArch::UNKNOWN;
   return result->arch;


Index: llvm/lib/Support/TargetParser.cpp
===
--- llvm/lib/Support/TargetParser.cpp
+++ llvm/lib/Support/TargetParser.cpp
@@ -151,7 +151,7 @@
 
 AMDGPU::GPUKind llvm::AMDGPU::parseArchAMDGCN(StringRef CPU) {
   for (const auto  : AMDGCNGPUs) {
-if (CPU == C.Name)
+if (CPU.equals_insensitive(C.Name))
   return C.Kind;
   }
 
Index: clang/test/Driver/openmp-offload-infer.c
===
--- clang/test/Driver/openmp-offload-infer.c
+++ clang/test/Driver/openmp-offload-infer.c
@@ -18,6 +18,9 @@
 // RUN:   %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings -fopenmp \
 // RUN: --offload-arch=sm_70 --offload-arch=gfx908:sramecc+:xnack- \
 // RUN: -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-NVIDIA-AMDGPU
+// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings -fopenmp \
+// RUN: --offload-arch=SM_70 --offload-arch=GFX908:sramecc+:xnack- \
+// RUN: -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-NVIDIA-AMDGPU
 
 // CHECK-NVIDIA-AMDGPU: "x86_64-unknown-linux-gnu" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[HOST_BC:.+]]"
 // CHECK-NVIDIA-AMDGPU: "amdgcn-amd-amdhsa" - "clang", inputs: ["[[INPUT]]", "[[HOST_BC]]"], output: "[[AMD_BC:.+]]"
Index: clang/lib/Basic/Cuda.cpp
===
--- clang/lib/Basic/Cuda.cpp
+++ clang/lib/Basic/Cuda.cpp
@@ -155,9 +155,10 @@
 }
 
 CudaArch StringToCudaArch(llvm::StringRef S) {
-  auto result = std::find_if(
-  std::begin(arch_names), std::end(arch_names),
-  [S](const CudaArchToStringMap ) { return S == map.arch_name; });
+  auto result = std::find_if(std::begin(arch_names), std::end(arch_names),
+ [S](const CudaArchToStringMap ) {
+   return S.equals_insensitive(map.arch_name);
+ });
   if (result == std::end(arch_names))
 return CudaArch::UNKNOWN;