Jump to conversation
Unresolved conversations (5)
@sam-mccall sam-mccall Sep 27, 2023
This header is not part of `clangBasic`'s interface, but rather its implementation (`lib/Basic` rather than `include/clang/Basic`). Sema can't depend on it - if you need to use its APIs from outside clangBasic they should be moved to a public header. The bazel build shows the problem: https://buildkite.com/llvm-project/upstream-bazel/builds/75928#018ad568-2f7c-4dda-ae90-3b4d787caad7. Breaking bazel is not itself reason to revert, but here it's flagging a real problem that CMake doesn't catch. I've reverted as 0afbcb20fd908f8bf9073697423da097be7db592 - sorry to do this so abruptly, but I can't fix this myself & such problems block downstream use of LLVM.
clang/lib/Sema/SemaDeclAttr.cpp
sam-mccall jchlanda
@Artem-B Artem-B Sep 21, 2023
Do we want to ignore this directive if the metadata exists, but we're targeting a pre-sm_90 GPU? It may be useful for non-clang LLVM users (e.g XLA) to be able to always specify launch bounds metadata, and let LLVM decide on what it can do with it. Generating the directive for older GPUs would result in ptxas error, while ignoring it would still allow the kernels to compile and work, the same as would be the case if the metadata was correctly absent. I don't think there's not much point to require users to jump through more hoops just to achieve exactly the same result.
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
jchlanda Artem-B
@Artem-B Artem-B Sep 21, 2023
Considering that we do have TargetInfo pointer here, instead of trying to figure out the target GPU via features, can we just extract CudaArch directly from [NVPTXTargetInfo::GPU](https://github.com/llvm/llvm-project/blob/7fcbb64fca5e664b11f9167a215b47de2ae9082c/clang/lib/Basic/Targets/NVPTX.h#L65C3-L65C12) ?
clang/lib/Sema/SemaDeclAttr.cpp
jchlanda Artem-B
@Artem-B Artem-B Sep 21, 2023
Are we ignoring the whole launch_bounds attribute, or only the MaxBlocks parameter?
...nclude/clang/Basic/DiagnosticSemaKinds.td
jchlanda
@ldrumm ldrumm Sep 21, 2023
Do we have enough information to assert this is non-negative?
clang/lib/CodeGen/Targets/NVPTX.cpp
jchlanda
Resolved conversations (6)
@Artem-B Artem-B Sep 21, 2023
Same here -- need to check the negative max bounds, too.
clang/test/SemaCUDA/launch_bounds_sm_90.cu
jchlanda
@Artem-B Artem-B Sep 21, 2023
We need a test for negative max blocks argument.
clang/test/SemaCUDA/launch_bounds.cu
jchlanda
@Artem-B Artem-B Sep 21, 2023
Nit: `.maxclusterrank` is a PTX directive.
Outdated
clang/lib/Sema/SemaDeclAttr.cpp
@ldrumm ldrumm Sep 21, 2023
Is there a missing negation here? Also, as above: (modulo negation) ```suggestion MaxSpecified |= getMaxNTIDx(F, Maxntidx); MaxSpecified |= !getMaxNTIDy(F, Maxntidy); MaxSpecified |= !getMaxNTIDz(F, Maxntidz); ```
Outdated
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
jchlanda
@ldrumm ldrumm Sep 21, 2023
I think I know what you were going for with those ors: ```suggestion ReqSpecified |= getReqNTIDx(F, Reqntidx); ReqSpecified |= getReqNTIDy(F, Reqntidy); ReqSpecified |= getReqNTIDz(F, Reqntidz); ```
Outdated
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
@ldrumm ldrumm Sep 21, 2023
```suggestion if (!MaxBlocks) ```
Outdated
clang/lib/Sema/SemaDeclAttr.cpp
jchlanda