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

Thu Oct 13 16:25:08 PDT 2022

jdoerfert added a comment.

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

In D135614#3847775 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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