Also check if native half types are supported to give more descriptive error message, without it clang only reports incorrect intrinsic return type.
Details
Diff Detail
Unit Tests
Event Timeline
Nice. Thank you!
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
18104 | I'd add a comment describing a meaning of the fields in the returned pair. | |
18116 | Can we use the standard StringRef Name = getContext().BuiltinInfo.getName(BuiltinID); to figure out the builtin name? | |
18261–18263 | Nit: this would be a bit more readable: std::string BuiltinName{HalfSupport.second}; CGM.Error(E->getExprLoc(), BuiltinName + " requires native half type support.")` You may also consider changing returned BuiltinName to be std::string, so you would not need an explicit temp var. | |
clang/test/CodeGen/builtins-nvptx-native-half-type-err.c | ||
3 | I think we could've done it all in one run if we were to do both ldg and ldu in one function. | |
llvm/test/CodeGen/NVPTX/ldu-ldg.ll | ||
33 | Should alignment be 2 ? | |
60 | Hmm. I wonder why we end up with u64 here and not b64. Not that it matters in this case, but it is a discrepancy vs. f16. |
Address PR comments
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
18116 | Ha, had a feeling it would exist, couldn't find it. Thanks. | |
18261–18263 | Done the std::string return, thanks. | |
llvm/test/CodeGen/NVPTX/ldu-ldg.ll | ||
60 | That is copy/paste sloppiness on my part, sorry. I've updated the test to check generated PTX, not just the labels, and fixed the values. It generates correct kinds of loads, based on the type, the only discrepancy is that it doesn't distinguish between signed and unsigned loads, always choosing the unsigned variant. I think that's by design, at ISel there is a check if the load needs to be extended and a correct CVT instruction will be added. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
18259–18263 | I think we can simplify it all further. auto HasHalfSupport = [&](unsigned BuiltinID) { auto &Context = getContext(); return Context.getLangOpts().NativeHalfType || !Context.getTargetInfo().useFP16ConversionIntrinsics(); } ... if (!HasHalfSupport(BuiltinID)) { CGM.Error(E->getExprLoc(), getContext().BuiltinInfo.getName(BuiltinID) + " requires native half type support."); |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
18259–18263 | Done, although we need a string append there, StringRef and Twine would not work. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
18259–18263 | We don't really need append(). |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
18259–18263 | Done. |
LGTM with a test nit.
llvm/test/CodeGen/NVPTX/ldu-ldg.ll | ||
---|---|---|
30 | Nit. Function names usually checked with CHECK-LABEL. |
I'd add a comment describing a meaning of the fields in the returned pair.