[PATCH] D135328: [CUDA] Refactored CUDA version housekeeping to use less boilerplate.

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/lib/Basic/Cuda.cpp:59
+CudaVersion ToCudaVersion(llvm::VersionTuple Version) {
+  int IVer = Version.getMajor() * 10 + Version.getMinor().value_or(0);
+  for (auto *I = CudaNameVersionMap; I->Version != CudaVersion::UNKNOWN; ++I)
----------------
yaxunl wrote:
> should we assert Version.getMinor().value_or(0)<10 ?
It's not an immediate issue, but you are correct that we may potentially have CUDA 12.34 and that will mess up the integer version encoding. 

In fact, NVIDIA hit exactly this kind of problem in CUDA-11.7 when they had to version some of the libraries as 11.10 and had to change the binary representation and break existing ABIs. Here we're only dealing with internal use, but I'll update the encoding to give us more wiggle room.


================
Comment at: clang/lib/Basic/Targets/NVPTX.cpp:45-46
+    int PTXV;
+    if (!Feature.startswith("+ptx") ||
+        Feature.drop_front(4).getAsInteger(10, PTXV))
       continue;
----------------
yaxunl wrote:
> This behaves differently when seeing a +ptx value not listed in the original code. Previously, it returns 32, now it will just return that value. Is this intended?
Indeed, previously it would set PTXVersion=32 if the specific ptx feature was not listed. I believe it was a bug as it would clobber correctly parsed features before it.

Right now we will only update PTXVersion, if we did manage to parse the feature. The code looks a bit misleading. `getAsInteger()` returns `true` on failure and when that happens, we hit `continue` w/o setting PTXVersion.

I was considering erroring out here, but that would be a problem if we were to have another feature starting with `+ptx`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135328