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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
