[CIR] Upstream basic alloca and load support #128792
Conversation
This change implements basic support in ClangIR for local variables using the cir.alloca and cir.load operations.
andykaylor
requested review from
AaronBallman,
bcardosolopes,
erichkeane,
lanza,
AmrDeveloper and
dkolsen-pgi
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesThis change implements basic support in ClangIR for local variables using the cir.alloca and cir.load operations. Patch is 42.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128792.diff 18 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
index f03241a875845..a62b00cc8ed33 100644
--- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
+++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_CIR_DIALECT_BUILDER_CIRBASEBUILDER_H
#define LLVM_CLANG_CIR_DIALECT_BUILDER_CIRBASEBUILDER_H
+#include "clang/AST/CharUnits.h"
#include "clang/CIR/Dialect/IR/CIRAttrs.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
#include "clang/CIR/Dialect/IR/CIRTypes.h"
@@ -51,6 +52,49 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
return cir::ConstPtrAttr::get(
getContext(), mlir::cast<cir::PointerType>(type), valueAttr);
}
+
+ mlir::Value createAlloca(mlir::Location loc, cir::PointerType addrType,
+ mlir::Type type, llvm::StringRef name,
+ mlir::IntegerAttr alignment,
+ mlir::Value dynAllocSize) {
+ return create<cir::AllocaOp>(loc, addrType, type, name, alignment,
+ dynAllocSize);
+ }
+
+ cir::LoadOp createLoad(mlir::Location loc, mlir::Value ptr,
+ bool isVolatile = false, uint64_t alignment = 0) {
+ mlir::IntegerAttr intAttr;
+ if (alignment)
+ intAttr = mlir::IntegerAttr::get(
+ mlir::IntegerType::get(ptr.getContext(), 64), alignment);
+
+ return create<cir::LoadOp>(loc, ptr);
+ }
+
+ //
+ // Block handling helpers
+ // ----------------------
+ //
+ static OpBuilder::InsertPoint getBestAllocaInsertPoint(mlir::Block *block) {
+ auto last =
+ std::find_if(block->rbegin(), block->rend(), [](mlir::Operation &op) {
+ // TODO: Add LabelOp missing feature here
+ return mlir::isa<cir::AllocaOp>(&op);
+ });
+
+ if (last != block->rend())
+ return OpBuilder::InsertPoint(block, ++mlir::Block::iterator(&*last));
+ return OpBuilder::InsertPoint(block, block->begin());
+ };
+
+ mlir::IntegerAttr getSizeFromCharUnits(mlir::MLIRContext *ctx,
+ clang::CharUnits size) {
+ // Note that mlir::IntegerType is used instead of cir::IntType here
+ // because we don't need sign information for this to be useful, so keep
+ // it simple.
+ return mlir::IntegerAttr::get(mlir::IntegerType::get(ctx, 64),
+ size.getQuantity());
+ }
};
} // namespace cir
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
index 097616ba06749..ece04c225e322 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
@@ -54,6 +54,21 @@ def CIR_BoolAttr : CIR_Attr<"Bool", "bool", [TypedAttrInterface]> {
}];
}
+//===----------------------------------------------------------------------===//
+// UndefAttr
+//===----------------------------------------------------------------------===//
+
+def UndefAttr : CIR_Attr<"Undef", "undef", [TypedAttrInterface]> {
+ let summary = "Represent an undef constant";
+ let description = [{
+ The UndefAttr represents an undef constant, corresponding to LLVM's notion
+ of undef.
+ }];
+
+ let parameters = (ins AttributeSelfTypeParameter<"">:$type);
+ let assemblyFormat = [{}];
+}
+
//===----------------------------------------------------------------------===//
// IntegerAttr
//===----------------------------------------------------------------------===//
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 45d39807b35c8..1ff5e215c10a0 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -115,6 +115,149 @@ def ConstantOp : CIR_Op<"const",
let hasFolder = 1;
}
+//===----------------------------------------------------------------------===//
+// AllocaOp
+//===----------------------------------------------------------------------===//
+
+class AllocaTypesMatchWith<string summary, string lhsArg, string rhsArg,
+ string transform, string comparator = "std::equal_to<>()">
+ : PredOpTrait<summary, CPred<
+ comparator # "(" #
+ !subst("$_self", "$" # lhsArg # ".getType()", transform) #
+ ", $" # rhsArg # ")">> {
+ string lhs = lhsArg;
+ string rhs = rhsArg;
+ string transformer = transform;
+}
+
+def AllocaOp : CIR_Op<"alloca", [
+ AllocaTypesMatchWith<"'allocaType' matches pointee type of 'addr'",
+ "addr", "allocaType",
+ "cast<PointerType>($_self).getPointee()">,
+ DeclareOpInterfaceMethods<PromotableAllocationOpInterface>]> {
+ let summary = "Defines a scope-local variable";
+ let description = [{
+ The `cir.alloca` operation defines a scope-local variable.
+
+ The presence `init` attribute indicates that the local variable represented
+ by this alloca was originally initialized in C/C++ source code. In such
+ cases, the first use contains the initialization (a cir.store, a cir.call
+ to a ctor, etc).
+
+ The presence of the `const` attribute indicates that the local variable is
+ declared with C/C++ `const` keyword.
+
+ The `dynAllocSize` specifies the size to dynamically allocate on the stack
+ and ignores the allocation size based on the original type. This is useful
+ when handling VLAs and is omitted when declaring regular local variables.
+
+ The result type is a pointer to the input's type.
+
+ Example:
+
+ ```mlir
+ // int count = 3;
+ %0 = cir.alloca i32, !cir.ptr<i32>, ["count", init] {alignment = 4 : i64}
+
+ // int *ptr;
+ %1 = cir.alloca !cir.ptr<i32>, !cir.ptr<!cir.ptr<i32>>, ["ptr"] {alignment = 8 : i64}
+ ...
+ ```
+ }];
+
+ let arguments = (ins
+ Optional<PrimitiveInt>:$dynAllocSize,
+ TypeAttr:$allocaType,
+ StrAttr:$name,
+ UnitAttr:$init,
+ UnitAttr:$constant,
+ ConfinedAttr<OptionalAttr<I64Attr>, [IntMinValue<0>]>:$alignment,
+ OptionalAttr<ArrayAttr>:$annotations
+ );
+
+ let results = (outs Res<CIR_PointerType, "",
+ [MemAlloc<AutomaticAllocationScopeResource>]>:$addr);
+
+ let skipDefaultBuilders = 1;
+ let builders = [
+ OpBuilder<(ins "mlir::Type":$addr, "mlir::Type":$allocaType,
+ "llvm::StringRef":$name,
+ "mlir::IntegerAttr":$alignment)>,
+
+ OpBuilder<(ins "mlir::Type":$addr,
+ "mlir::Type":$allocaType,
+ "llvm::StringRef":$name,
+ "mlir::IntegerAttr":$alignment,
+ "mlir::Value":$dynAllocSize),
+ [{
+ if (dynAllocSize)
+ $_state.addOperands(dynAllocSize);
+ build($_builder, $_state, addr, allocaType, name, alignment);
+ }]>
+ ];
+
+ let extraClassDeclaration = [{
+ // Whether the alloca input type is a pointer.
+ bool isPointerType() { return ::mlir::isa<::cir::PointerType>(getAllocaType()); }
+
+ bool isDynamic() { return (bool)getDynAllocSize(); }
+ }];
+
+ let assemblyFormat = [{
+ $allocaType `,` qualified(type($addr)) `,`
+ ($dynAllocSize^ `:` type($dynAllocSize) `,`)?
+ `[` $name
+ (`,` `init` $init^)?
+ (`,` `const` $constant^)?
+ `]`
+ ($annotations^)? attr-dict
+ }];
+
+ let hasVerifier = 0;
+}
+
+//===----------------------------------------------------------------------===//
+// LoadOp
+//===----------------------------------------------------------------------===//
+
+def LoadOp : CIR_Op<"load", [
+ TypesMatchWith<"type of 'result' matches pointee type of 'addr'",
+ "addr", "result",
+ "cast<PointerType>($_self).getPointee()">,
+ DeclareOpInterfaceMethods<PromotableMemOpInterface>]> {
+
+ let summary = "Load value from memory adddress";
+ let description = [{
+ `cir.load` reads a value (lvalue to rvalue conversion) given an address
+ backed up by a `cir.ptr` type. A unit attribute `deref` can be used to
+ mark the resulting value as used by another operation to dereference
+ a pointer. A unit attribute `volatile` can be used to indicate a volatile
+ loading. Load can be marked atomic by using `atomic(<mem_order>)`.
+
+ `align` can be used to specify an alignment that's different from the
+ default, which is computed from `result`'s type ABI data layout.
+
+ Example:
+
+ ```mlir
+
+ // Read from local variable, address in %0.
+ %1 = cir.load %0 : !cir.ptr<i32>, i32
+ ```
+ }];
+
+ let arguments = (ins Arg<CIR_PointerType, "the address to load from",
+ [MemRead]>:$addr
+ );
+ let results = (outs CIR_AnyType:$result);
+
+ let assemblyFormat = [{
+ $addr `:` qualified(type($addr)) `,` type($result) attr-dict
+ }];
+
+ // FIXME: add verifier.
+}
+
//===----------------------------------------------------------------------===//
// ReturnOp
//===----------------------------------------------------------------------===//
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index d4fcd52e7e6e3..97e919b5c2d74 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -36,6 +36,24 @@ struct MissingFeatures {
static bool opGlobalConstant() { return false; }
static bool opGlobalAlignment() { return false; }
static bool opGlobalLinkage() { return false; }
+
+ // Load attributes
+ static bool opLoadThreadLocal() { return false; }
+ static bool opLoadEmitScalarRangeCheck() { return false; }
+ static bool opLoadBooleanRepresentation() { return false; }
+
+ // AllocaOp handling
+ static bool opAllocaVarDeclContext() { return false; }
+ static bool opAllocaStaticLocal() { return false; }
+ static bool opAllocaNonGC() { return false; }
+ static bool opAllocaImpreciseLifetime() { return false; }
+ static bool opAllocaTLS() { return false; }
+ static bool opAllocaOpenMPThreadPrivate() { return false; }
+ static bool opAllocaEscapeByReference() { return false; }
+ static bool opAllocaReference() { return false; }
+
+ // Options for casts
+ static bool scalarConversionOpts() { return false; }
};
} // namespace cir
diff --git a/clang/lib/CIR/CodeGen/Address.h b/clang/lib/CIR/CodeGen/Address.h
new file mode 100644
index 0000000000000..c68facde9a18c
--- /dev/null
+++ b/clang/lib/CIR/CodeGen/Address.h
@@ -0,0 +1,76 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This class provides a simple wrapper for a pair of a pointer and an
+// alignment.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_CIR_ADDRESS_H
+#define LLVM_CLANG_LIB_CIR_ADDRESS_H
+
+#include "mlir/IR/Value.h"
+#include "clang/AST/CharUnits.h"
+#include "clang/CIR/Dialect/IR/CIRTypes.h"
+#include "llvm/ADT/PointerIntPair.h"
+
+namespace clang::CIRGen {
+
+class Address {
+
+ // The boolean flag indicates whether the pointer is known to be non-null.
+ llvm::PointerIntPair<mlir::Value, 1, bool> pointerAndKnownNonNull;
+
+ /// The expected CIR type of the pointer. Carrying accurate element type
+ /// information in Address makes it more convenient to work with Address
+ /// values and allows frontend assertions to catch simple mistakes.
+ mlir::Type elementType;
+
+ clang::CharUnits alignment;
+
+protected:
+ Address(std::nullptr_t) : elementType(nullptr) {}
+
+public:
+ Address(mlir::Value pointer, mlir::Type elementType,
+ clang::CharUnits alignment)
+ : pointerAndKnownNonNull(pointer, false), elementType(elementType),
+ alignment(alignment) {
+ assert(mlir::isa<cir::PointerType>(pointer.getType()) &&
+ "Expected cir.ptr type");
+
+ assert(pointer && "Pointer cannot be null");
+ assert(elementType && "Element type cannot be null");
+ assert(!alignment.isZero() && "Alignment cannot be zero");
+
+ assert(mlir::cast<cir::PointerType>(pointer.getType()).getPointee() ==
+ elementType);
+ }
+
+ static Address invalid() { return Address(nullptr); }
+ bool isValid() const {
+ return pointerAndKnownNonNull.getPointer() != nullptr;
+ }
+
+ mlir::Value getPointer() const {
+ assert(isValid());
+ return pointerAndKnownNonNull.getPointer();
+ }
+
+ mlir::Type getElementType() const {
+ assert(isValid());
+ assert(mlir::cast<cir::PointerType>(
+ pointerAndKnownNonNull.getPointer().getType())
+ .getPointee() == elementType);
+ return elementType;
+ }
+};
+
+} // namespace clang::CIRGen
+
+#endif // LLVM_CLANG_LIB_CIR_ADDRESS_H
diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
new file mode 100644
index 0000000000000..34e0b18594efe
--- /dev/null
+++ b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
@@ -0,0 +1,82 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This contains code to emit Decl nodes as CIR code.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CIRGenFunction.h"
+#include "clang/AST/Expr.h"
+#include "clang/CIR/MissingFeatures.h"
+
+using namespace clang;
+using namespace clang::CIRGen;
+
+/// Emit code and set up symbol table for a variable declaration with auto,
+/// register, or no storage class specifier. These turn into simple stack
+/// objects, globals depending on target.
+void CIRGenFunction::emitAutoVarDecl(const VarDecl &d) {
+ QualType ty = d.getType();
+ assert(ty.getAddressSpace() == LangAS::Default);
+
+ auto loc = getLoc(d.getSourceRange());
+
+ if (d.isEscapingByref())
+ cgm.errorNYI(d.getSourceRange(),
+ "emitAutoVarDecl: decl escaping by reference");
+
+ CharUnits alignment = getContext().getDeclAlign(&d);
+
+ // If the type is variably-modified, emit all the VLA sizes for it.
+ if (ty->isVariablyModifiedType())
+ cgm.errorNYI(d.getSourceRange(), "emitAutoVarDecl: variably modified type");
+
+ Address address = Address::invalid();
+ if (!ty->isConstantSizeType())
+ cgm.errorNYI(d.getSourceRange(), "emitAutoVarDecl: non-constant size type");
+
+ // A normal fixed sized variable becomes an alloca in the entry block,
+ mlir::Type allocaTy = convertTypeForMem(ty);
+ // Create the temp alloca and declare variable using it.
+ mlir::Value addrVal;
+ address = createTempAlloca(allocaTy, alignment, loc, d.getName(),
+ /*ArraySize=*/nullptr);
+ setAddrOfLocalVar(&d, address);
+ // TODO: emit var init and cleanup
+}
+
+void CIRGenFunction::emitVarDecl(const VarDecl &d) {
+ if (d.hasExternalStorage()) {
+ // Don't emit it now, allow it to be emitted lazily on its first use.
+ return;
+ }
+
+ if (d.getStorageDuration() != SD_Automatic)
+ cgm.errorNYI(d.getSourceRange(), "emitVarDecl automatic storage duration");
+ if (d.getType().getAddressSpace() == LangAS::opencl_local)
+ cgm.errorNYI(d.getSourceRange(), "emitVarDecl openCL address space");
+
+ assert(d.hasLocalStorage());
+
+ assert(!cir::MissingFeatures::opAllocaVarDeclContext());
+ return emitAutoVarDecl(d);
+}
+
+void CIRGenFunction::emitDecl(const Decl &d) {
+ switch (d.getKind()) {
+ case Decl::Var: {
+ const VarDecl &vd = cast<VarDecl>(d);
+ assert(vd.isLocalVarDecl() &&
+ "Should not see file-scope variables inside a function!");
+ emitVarDecl(vd);
+ return;
+ }
+ default:
+ cgm.errorNYI(d.getSourceRange(), "emitDecl: unhandled decl type");
+ }
+}
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
new file mode 100644
index 0000000000000..8bb670d0d5bae
--- /dev/null
+++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -0,0 +1,128 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This contains code to emit Expr nodes as CIR code.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Address.h"
+#include "CIRGenFunction.h"
+#include "CIRGenValue.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/CharUnits.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/CIR/Dialect/IR/CIRDialect.h"
+#include "clang/CIR/MissingFeatures.h"
+
+using namespace clang;
+using namespace clang::CIRGen;
+using namespace cir;
+
+mlir::Value CIRGenFunction::emitLoadOfScalar(LValue lvalue,
+ SourceLocation loc) {
+ assert(!cir::MissingFeatures::opLoadThreadLocal());
+ assert(!cir::MissingFeatures::opLoadEmitScalarRangeCheck());
+ assert(!cir::MissingFeatures::opLoadBooleanRepresentation());
+
+ Address addr = lvalue.getAddress();
+ mlir::Type eltTy = addr.getElementType();
+
+ auto ptr = addr.getPointer();
+ if (mlir::isa<cir::VoidType>(eltTy))
+ cgm.errorNYI(loc, "emitLoadOfScalar: void type");
+
+ auto loadOp = builder.CIRBaseBuilderTy::createLoad(getLoc(loc), ptr,
+ false /*isVolatile*/);
+
+ return loadOp;
+}
+
+/// Given an expression that represents a value lvalue, this
+/// method emits the address of the lvalue, then loads the result as an rvalue,
+/// returning the rvalue.
+RValue CIRGenFunction::emitLoadOfLValue(LValue lv, SourceLocation loc) {
+ assert(!lv.getType()->isFunctionType());
+ assert(!(lv.getType()->isConstantMatrixType()) && "not implemented");
+
+ if (lv.isSimple())
+ return RValue::get(emitLoadOfScalar(lv, loc));
+
+ cgm.errorNYI(loc, "emitLoadOfLValue");
+}
+
+LValue CIRGenFunction::emitDeclRefLValue(const DeclRefExpr *e) {
+ const NamedDecl *nd = e->getDecl();
+ QualType ty = e->getType();
+
+ assert(e->isNonOdrUse() != NOUR_Unevaluated &&
+ "should not emit an unevaluated operand");
+
+ if (const auto *vd = dyn_cast<VarDecl>(nd)) {
+ // Checks for omitted feature handling
+ assert(!cir::MissingFeatures::opAllocaStaticLocal());
+ assert(!cir::MissingFeatures::opAllocaNonGC());
+ assert(!cir::MissingFeatures::opAllocaImpreciseLifetime());
+ assert(!cir::MissingFeatures::opAllocaTLS());
+ assert(!cir::MissingFeatures::opAllocaOpenMPThreadPrivate());
+ assert(!cir::MissingFeatures::opAllocaEscapeByReference());
+
+ // Check if this is a global variable
+ if (vd->hasLinkage() || vd->isStaticDataMember())
+ cgm.errorNYI(vd->getSourceRange(), "emitDeclRefLValue: global variable");
+
+ Address addr = Address::invalid();
+
+ // The variable should generally be present in the local decl map.
+ auto iter = LocalDeclMap.find(vd);
+ if (iter != LocalDeclMap.end()) {
+ addr = iter->second;
+ } else {
+ // Otherwise, it might be static local we haven't emitted yet for some
+ // reason; most likely, because it's in an outer function.
+ cgm.errorNYI(vd->getSourceRange(), "emitDeclRefLValue: static local");
+ }
+
+ return LValue::makeAddr(addr, ty);
+ }
+
+ cgm.errorNYI(e->getSourceRange(), "emitDeclRefLValue: unhandled decl type");
+}
+
+mlir::Value CIRGenFunction::emitAlloca(StringRef name, mlir::Type ty,
+ mlir::Location loc, CharUnits alignment,
+ mlir::Value arraySize) {
+ mlir::Block *entryBlock = getCurFunctionEntryBlock();
+
+ // CIR uses its own alloca AS rather than follow the target data layout like
+ // original CodeGen. The data layout awareness should be done in the lowering
+ // pass instead.
+ a...
[truncated]
|
clang/test/CIR/CodeGen/basic.cpp
Outdated
| @@ -0,0 +1,13 @@ | |||
| // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - | FileCheck %s | |||
|
|
|||
| int f1(); | |||
There was a problem hiding this comment.
Silly question: What is the purpose here for the forward declaration of f1?
There was a problem hiding this comment.
Probably no purpose. It was in the incubator test that I'm starting to bring over here. I had taken it out, but I had some problems during development so I put it back in to see if that helped. The problem turned out to be related to handling of C versus C++ code, and I forgot to remove it after I straightened that out.
clang/lib/CIR/CodeGen/Address.h
Outdated
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #ifndef LLVM_CLANG_LIB_CIR_ADDRESS_H |
There was a problem hiding this comment.
Where did we fall on using the 'LLVM_' in header guards? I thought we weren't doing it anymore?
There was a problem hiding this comment.
OK. I wasn't sure about that.
clang/lib/CIR/CodeGen/CIRGenDecl.cpp
Outdated
| /// objects, globals depending on target. | ||
| void CIRGenFunction::emitAutoVarDecl(const VarDecl &d) { | ||
| QualType ty = d.getType(); | ||
| assert(ty.getAddressSpace() == LangAS::Default); |
There was a problem hiding this comment.
Is this a 'not yet implemented' sorta thing?
There was a problem hiding this comment.
Sort of? The assertion in the incubator asserts that the address space is default or private or OpenCL private. An awful lot of variations go through this path in the eventual implementation. I've ended up with something that is a bit inconsistent in terms of how it's handling errorNYI versus MissingFeatures versus just leaving things out.
Has there been a discussion on this previously? The incubator uses a lot of llvm_unreachable("NYI") but the upstreaming seems to be a mix of errorNYI and silently ignoring things. I think the goal should be to leave markers that make it easiest to find the point where new code should be inserted, which for me would mean llvm_unreachable("NYI") but there is so much that is NYI at this point that the code would be a very ugly mess.
There was a problem hiding this comment.
Guidance here was to use the errorNYI instead of 'unreachable', as 'unreachable' is a much less 'nice' user interface. So we leave unreachable for things that are NEVER going to be reachable.
There was a problem hiding this comment.
OK. I like unreachable because the diagnostic points me to the place in the code where the implementation is missing rather than the place in the user source, which is probably a test case that I just added. But I'm happy to go along with the consensus.
There was a problem hiding this comment.
upstreaming seems to be a mix of errorNYI and silently ignoring things
If upstream is silently ignoring things this is pretty bad, because it leads to subtle miscompilations that are super hard to debug/find. My understanding is that errorNYI should be covering all cases where we use unrecheable in the incubator, but if that isn't enough, unrecheable is 1000x better than silently ignoring
There was a problem hiding this comment.
Ideally, every not-yet-implemented or not-yet-upstreamed language construct should result in a user error, which is most easily done by calling errorNYI. In the incubator, those situations usually have an llvm_unreachable("NYI") or an assert(condition && "NYI"). We would prefer that upstream use errorNYI instead. If there is code where reporting a user error is not feasible (I am not familiar enough with Clang or MLIR to know how often that will happen), then an llvm_unreachable("NYI: <unimplemented feature>") will have to do.
It would be great if every unimplemented language construct was checked for and resulted in an error, but I don't think that's feasible at the moment. The number of unimplemented attributes and attribute-like properties that can be attached to declarations is huge, and adding a check for every one of them would be tedious and would overwhelm the happy-path code. As long as the upstreamed ClangIR is unable to compile anything useful, I think this is okay. Right now no one will compile real code and be puzzled at some subtle behavior change due to an ignored attribute. Adding all the checks for unimplemented attributes will be important when upstreamed ClangIR is able to compile real code, and by that time many of the attributes will already be implemented and won't need a temporary NYI check.
There was a problem hiding this comment.
If upstream is silently ignoring things this is pretty bad, because it leads to subtle miscompilations that are super hard to debug/find.
There's nothing really subtle about no CIR being generated for function arguments (the case I had in mind that's being ignored currently). I take your point though, and I'll try to incorporate the suggestion.
My question is more like this. When we have large sections of code being omitted that are checking for conditions that maybe can't even be checked yet because the support functions are missing, how should that be handled? In this function, I have checks plus errorNYI for a lot of things, but it feels very messy. What I really want is something like a single check for the limited cases we are handling, and an error for anything else, but that's not always possible. In places where the conditions can't be checked, I've put MissingFeature calls.
There was a problem hiding this comment.
In places where the conditions can't be checked, I've put MissingFeature calls.
Gotcha! Yes, I think this is the best we can do in those scenarios!
| CastKind kind = ce->getCastKind(); | ||
|
|
||
| // Since almost all cast kinds apply to scalars, this switch doesn't have a | ||
| // default case, so the compiler will warn on a missing case. The cases are |
There was a problem hiding this comment.
Er... DOESN'T it have a default case?
There was a problem hiding this comment.
Oops. I kept the comment from the original code, which doesn't have a default.
| @@ -49,13 +53,24 @@ class CIRGenFunction : public CIRGenTypeCache { | |||
| /// for. | |||
| mlir::Operation *curFn = nullptr; | |||
|
|
|||
| using DeclMapTy = llvm::DenseMap<const clang::Decl *, Address>; | |||
| /// This keeps track of the CIR allocas or globals for local C | |||
| /// delcs. | |||
There was a problem hiding this comment.
| /// delcs. | |
| /// declarations. |
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
| mlir::LogicalResult CIRGenFunction::emitDeclStmt(const DeclStmt &s) { | ||
| assert(builder.getInsertionBlock() && "expected valid insertion point"); | ||
|
|
||
| for (const auto *I : s.decls()) { |
There was a problem hiding this comment.
single-line curleys, also,
| for (const auto *I : s.decls()) { | |
| for (const Decl *I : s.decls()) { |
clang/lib/CIR/CodeGen/CIRGenValue.h
Outdated
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #ifndef LLVM_CLANG_LIB_CIR_CIRGENVALUE_H |
There was a problem hiding this comment.
Same question here as above.
| // AllocaOp | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| void cir::AllocaOp::build(::mlir::OpBuilder &odsBuilder, |
There was a problem hiding this comment.
Why the global scope operators here? Is this a different mlir namespace than elsewhere? And llvm?
There was a problem hiding this comment.
MLIR is so many levels deep in templates and auto-generated code here that it makes my head hurt. This function gets called from the call to create<cir::AllocaOp> in CIRBaseBuilder.h. This is a static member function in the cir::AllocaOp class that gets generated from CIROps.td, but only the declaration is generated, so we have to provide the definition here.
There was a problem hiding this comment.
Right, but ::mlir::OpBuilder instead of mlir::OpBuilder? The lines a few above don't need to do this, so i was wondering why.
There was a problem hiding this comment.
Oh! I misunderstood your original comment. This appears to be an MLIR style thing. Maybe @bcardosolopes or @lanza can provide more insight into the history of this. I don't think it's necessary.
There was a problem hiding this comment.
I don't think this is necessary either - we went back n forth with namespaces in the past couple years, looks like an oversight of some previous bulk change at this point.
|
|
||
| let arguments = (ins Arg<CIR_PointerType, "the address to load from", | ||
| [MemRead]>:$addr | ||
| ); |
There was a problem hiding this comment.
The closing paren probably belongs in the line above?
clang/lib/CIR/CodeGen/CIRGenDecl.cpp
Outdated
| /// objects, globals depending on target. | ||
| void CIRGenFunction::emitAutoVarDecl(const VarDecl &d) { | ||
| QualType ty = d.getType(); | ||
| assert(ty.getAddressSpace() == LangAS::Default); |
There was a problem hiding this comment.
upstreaming seems to be a mix of errorNYI and silently ignoring things
If upstream is silently ignoring things this is pretty bad, because it leads to subtle miscompilations that are super hard to debug/find. My understanding is that errorNYI should be covering all cases where we use unrecheable in the incubator, but if that isn't enough, unrecheable is 1000x better than silently ignoring
clang/lib/CIR/CodeGen/CIRGenDecl.cpp
Outdated
| address = createTempAlloca(allocaTy, alignment, loc, d.getName(), | ||
| /*ArraySize=*/nullptr); | ||
| setAddrOfLocalVar(&d, address); | ||
| // TODO: emit var init and cleanup |
There was a problem hiding this comment.
This needs a assert on missing features otherwise it gets hard to track
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This change implements basic support in ClangIR for local variables using the cir.alloca and cir.load operations.