[CIR][X86] Add NYI diagnostic for __builtin_ia32_sqrtps #168320
Conversation
Priyanshu3820
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Priyanshu Kumar (Priyanshu3820) Changes[CIR][X86] Add NYI diagnostic for CIR currently doesn't handle lowering of the x86 builtin A simple CIR test is added to check the diagnostic. Test: Full diff: https://github.com/llvm/llvm-project/pull/168320.diff 3 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp b/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp
index ba160373ec77e..b48a6575eddaf 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp
@@ -20,17 +20,16 @@
using namespace clang;
using namespace clang::CIRGen;
-template <typename... Operands>
+namespace {
static mlir::Value emitIntrinsicCallOp(CIRGenFunction &cgf, const CallExpr *e,
- const std::string &str,
- const mlir::Type &resTy,
- Operands &&...op) {
- CIRGenBuilderTy &builder = cgf.getBuilder();
- mlir::Location location = cgf.getLoc(e->getExprLoc());
- return cir::LLVMIntrinsicCallOp::create(builder, location,
- builder.getStringAttr(str), resTy,
- std::forward<Operands>(op)...)
- .getResult();
+ llvm::StringRef name,
+ mlir::Type resultType,
+ llvm::ArrayRef<mlir::Value> args = {}) {
+ cgf.getCIRGenModule().errorNYI(
+ e->getSourceRange(),
+ ("CIR intrinsic lowering NYI for " + name.str()).c_str());
+ return {};
+}
}
mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
@@ -55,9 +54,6 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
// Find out if any arguments are required to be integer constant expressions.
assert(!cir::MissingFeatures::handleBuiltinICEArguments());
- // The operands of the builtin call
- llvm::SmallVector<mlir::Value> ops;
-
// `ICEArguments` is a bitmap indicating whether the argument at the i-th bit
// is required to be a constant integer expression.
unsigned iceArguments = 0;
@@ -65,8 +61,10 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
getContext().GetBuiltinType(builtinID, error, &iceArguments);
assert(error == ASTContext::GE_None && "Error while getting builtin type.");
- for (auto [idx, arg] : llvm::enumerate(e->arguments()))
- ops.push_back(emitScalarOrConstFoldImmArg(iceArguments, idx, arg));
+ llvm::SmallVector<mlir::Value> ops;
+ ops.reserve(e->getNumArgs());
+ for (const Expr *arg : e->arguments())
+ ops.push_back(emitScalarExpr(arg));
CIRGenBuilderTy &builder = getBuilder();
mlir::Type voidTy = builder.getVoidTy();
@@ -75,7 +73,7 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
default:
return {};
case X86::BI_mm_clflush:
- return emitIntrinsicCallOp(*this, e, "x86.sse2.clflush", voidTy, ops[0]);
+ return emitIntrinsicCallOp(*this, e, "x86.sse2.clflush", voidTy, {ops[0]});
case X86::BI_mm_lfence:
return emitIntrinsicCallOp(*this, e, "x86.sse2.lfence", voidTy);
case X86::BI_mm_pause:
@@ -643,6 +641,10 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
case X86::BI__builtin_ia32_kunpckdi:
case X86::BI__builtin_ia32_kunpcksi:
case X86::BI__builtin_ia32_kunpckhi:
+ cgm.errorNYI(e->getSourceRange(),
+ std::string("unimplemented X86 builtin call: ") +
+ getContext().BuiltinInfo.getName(builtinID));
+ return {};
case X86::BI__builtin_ia32_sqrtsh_round_mask:
case X86::BI__builtin_ia32_sqrtsd_round_mask:
case X86::BI__builtin_ia32_sqrtss_round_mask:
@@ -650,6 +652,9 @@ mlir::Value CIRGenFunction::emitX86BuiltinExpr(unsigned builtinID,
case X86::BI__builtin_ia32_sqrtpd:
case X86::BI__builtin_ia32_sqrtps256:
case X86::BI__builtin_ia32_sqrtps:
+ cgm.errorNYI(e->getSourceRange(),
+ "CIR lowering for x86 sqrt builtins is not implemented yet");
+ return {};
case X86::BI__builtin_ia32_sqrtph256:
case X86::BI__builtin_ia32_sqrtph:
case X86::BI__builtin_ia32_sqrtph512:
diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
index 33f2de9137514..9bfdcea5ba275 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
@@ -21,6 +21,7 @@
#include "clang/CIR/MissingFeatures.h"
+#include<algorithm>
using namespace clang;
using namespace clang::CIRGen;
@@ -97,7 +98,7 @@ EHScopeStack::getInnermostActiveNormalCleanup() const {
char *EHScopeStack::allocate(size_t size) {
size = llvm::alignTo(size, ScopeStackAlignment);
if (!startOfBuffer) {
- unsigned capacity = llvm::PowerOf2Ceil(std::max(size, 1024ul));
+ unsigned capacity = llvm::PowerOf2Ceil(std::max(static_cast<unsigned long>(size), 1024ul));
startOfBuffer = std::make_unique<char[]>(capacity);
startOfData = endOfBuffer = startOfBuffer.get() + capacity;
} else if (static_cast<size_t>(startOfData - startOfBuffer.get()) < size) {
diff --git a/clang/test/CIR/CodeGen/X86/builtin-x86-sqrt.c b/clang/test/CIR/CodeGen/X86/builtin-x86-sqrt.c
new file mode 100644
index 0000000000000..3692643c219e4
--- /dev/null
+++ b/clang/test/CIR/CodeGen/X86/builtin-x86-sqrt.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir -o - %s 2>&1 | FileCheck %s
+
+// Minimal stand-in for the SSE vector type.
+typedef float __m128 __attribute__((__vector_size__(16)));
+
+// Declare the builtin explicitly so we don't need headers.
+__m128 __builtin_ia32_sqrtps(__m128);
+
+__m128 test_sqrtps(__m128 a) {
+ return __builtin_ia32_sqrtps(a);
+}
+
+// CHECK: error: ClangIR code gen Not Yet Implemented: CIR lowering for x86 sqrt builtins is not implemented yet
+// CHECK: error: ClangIR code gen Not Yet Implemented: unimplemented builtin call: __builtin_ia32_sqrtps
|
|
Hi @andykaylor @lanza @bcardosolopes @xlauko, |
andykaylor
left a comment
andykaylor
left a comment
There was a problem hiding this comment.
The actual implementation for this isn't very big. See
There's no real value in replacing the current diagnostic with a new one, as opposed to just implementing the handling.
| llvm::StringRef name, | ||
| mlir::Type resultType, | ||
| llvm::ArrayRef<mlir::Value> args = {}) { | ||
| cgf.getCIRGenModule().errorNYI( |
There was a problem hiding this comment.
You've got some unrelated changes here. I'm not sure why this happened.
There was a problem hiding this comment.
Thanks for pointing that out, that emitIntrinsicCallOp change was leftover from some local experimentation and isn’t needed for this patch.
I’ve reverted those unrelated changes so this PR now only adds the X86 sqrt / kunpck NYI diagnostics and the corresponding test.
I will follow up with a full implementation of __builtin_ia32_sqrtps in CIR including a dedicated CIR operation or reuse of an existing math op and DirectToLLVM lowering to llvm.sqrt.v4f32.
Should I close this PR?
|
We already have a diagnostic for missing X86 builtins. Why don't you try implementing the missing builtin instead? |
Priyanshu3820
force-pushed
the
cir-x86-sqrt-builtin
branch
from
621bdf9 to
f4139a9
Compare
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,c -- clang/test/CIR/CodeGen/X86/builtin-x86-sqrt.c clang/lib/CIR/CodeGen/CIRGenBuiltinX86.cpp clang/lib/CIR/CodeGen/CIRGenCleanup.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
index 9bfdcea5b..1b29f9cfd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp
@@ -21,7 +21,7 @@
#include "clang/CIR/MissingFeatures.h"
-#include<algorithm>
+#include <algorithm>
using namespace clang;
using namespace clang::CIRGen;
@@ -98,7 +98,8 @@ EHScopeStack::getInnermostActiveNormalCleanup() const {
char *EHScopeStack::allocate(size_t size) {
size = llvm::alignTo(size, ScopeStackAlignment);
if (!startOfBuffer) {
- unsigned capacity = llvm::PowerOf2Ceil(std::max(static_cast<unsigned long>(size), 1024ul));
+ unsigned capacity =
+ llvm::PowerOf2Ceil(std::max(static_cast<unsigned long>(size), 1024ul));
startOfBuffer = std::make_unique<char[]>(capacity);
startOfData = endOfBuffer = startOfBuffer.get() + capacity;
} else if (static_cast<size_t>(startOfData - startOfBuffer.get()) < size) {
|
|
@Priyanshu3820 thanks for your first contribution, unfortunately this is not needed (as others pointed out NYI message already points to what's missing). Looking fwd to the full implementation PR, closing this one! |
🐧 Linux x64 Test Results
All tests passed but another part of the build failed. Click on a failure below to see the details. tools/clang/lib/CIR/CodeGen/CMakeFiles/obj.clangCIR.dir/CIRGenBuiltinX86.cpp.oIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
5 participants
[CIR][X86] Add NYI diagnostic for
__builtin_ia32_sqrtpsCIR currently doesn't handle lowering of the x86 builtin
__builtin_ia32_sqrtps. This patch adds a "not yet implemented" errorinstead of letting CIR codegen fail later, making the missing support
explicit.
A simple CIR test is added to check the diagnostic.
Test:
clang/test/CIR/CodeGen/X86/builtin-x86-sqrt.c