Address review feedback

Author: andykaylor

clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h

@@ -55,10 +55,8 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
 
   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);
+                           mlir::IntegerAttr alignment) {
+    return create<cir::AllocaOp>(loc, addrType, type, name, alignment);
   }
 
   cir::LoadOp createLoad(mlir::Location loc, mlir::Value ptr,

clang/include/clang/CIR/Dialect/IR/CIROps.td

@@ -139,25 +139,16 @@ def AllocaOp : CIR_Op<"alloca", [
   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 count;
+    %0 = cir.alloca i32, !cir.ptr<i32>, ["count"] {alignment = 4 : i64}
 
     // int *ptr;
     %1 = cir.alloca !cir.ptr<i32>, !cir.ptr<!cir.ptr<i32>>, ["ptr"] {alignment = 8 : i64}
@@ -166,7 +157,6 @@ def AllocaOp : CIR_Op<"alloca", [
   }];
 
   let arguments = (ins
-    Optional<PrimitiveInt>:$dynAllocSize,
     TypeAttr:$allocaType,
     StrAttr:$name,
     UnitAttr:$init,
@@ -180,32 +170,19 @@ def AllocaOp : CIR_Op<"alloca", [
 
   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);
-    }]>
+                   "mlir::IntegerAttr":$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^)?
@@ -229,13 +206,7 @@ def LoadOp : CIR_Op<"load", [
   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.
+    backed up by a `cir.ptr` type.
 
     Example:
 
@@ -247,8 +218,7 @@ def LoadOp : CIR_Op<"load", [
   }];
 
   let arguments = (ins Arg<CIR_PointerType, "the address to load from",
-                           [MemRead]>:$addr
-                       );
+                           [MemRead]>:$addr);
   let results = (outs CIR_AnyType:$result);
 
   let assemblyFormat = [{

clang/include/clang/CIR/MissingFeatures.h

@@ -30,6 +30,9 @@ struct MissingFeatures {
   // This isn't needed until we add support for bools.
   static bool convertTypeForMemory() { return false; }
 
+  // CIRGenFunction implementation details
+  static bool cgfSymbolTable() { return false; }
+
   // Unhandled global/linkage information.
   static bool opGlobalDSOLocal() { return false; }
   static bool opGlobalThreadLocal() { return false; }
@@ -47,13 +50,15 @@ struct MissingFeatures {
   static bool opAllocaStaticLocal() { return false; }
   static bool opAllocaNonGC() { return false; }
   static bool opAllocaImpreciseLifetime() { return false; }
+  static bool opAllocaPreciseLifetime() { 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
+  // Misc
   static bool scalarConversionOpts() { return false; }
+  static bool tryEmitAsConstant() { return false; }
 };
 
 } // namespace cir

clang/lib/CIR/CodeGen/Address.h

@@ -11,8 +11,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_CLANG_LIB_CIR_ADDRESS_H
-#define LLVM_CLANG_LIB_CIR_ADDRESS_H
+#ifndef CLANG_LIB_CIR_ADDRESS_H
+#define CLANG_LIB_CIR_ADDRESS_H
 
 #include "mlir/IR/Value.h"
 #include "clang/AST/CharUnits.h"
@@ -73,4 +73,4 @@ class Address {
 
 } // namespace clang::CIRGen
 
-#endif // LLVM_CLANG_LIB_CIR_ADDRESS_H
+#endif // CLANG_LIB_CIR_ADDRESS_H

clang/lib/CIR/CodeGen/CIRGenDecl.cpp

@@ -11,18 +11,18 @@
 //===----------------------------------------------------------------------===//
 
 #include "CIRGenFunction.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/Decl.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) {
+void CIRGenFunction::emitAutoVarAlloca(const VarDecl &d) {
   QualType ty = d.getType();
-  assert(ty.getAddressSpace() == LangAS::Default);
+  if (ty.getAddressSpace() != LangAS::Default)
+    cgm.errorNYI(d.getSourceRange(), "emitAutoVarAlloca: address space");
 
   auto loc = getLoc(d.getSourceRange());
 
@@ -43,18 +43,50 @@ void CIRGenFunction::emitAutoVarDecl(const VarDecl &d) {
   // 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);
+  address = createTempAlloca(allocaTy, alignment, loc, d.getName());
+  declare(address, &d, ty, getLoc(d.getSourceRange()), alignment);
+    
   setAddrOfLocalVar(&d, address);
-  // TODO: emit var init and cleanup
+}
+
+void CIRGenFunction::emitAutoVarInit(const clang::VarDecl &d) {
+  QualType type = d.getType();
+
+  // If this local has an initializer, emit it now.
+  const Expr *init = d.getInit();
+
+  if (init || !type.isPODType(getContext())) {
+    cgm.errorNYI(d.getSourceRange(), "emitAutoVarInit");
+  }
+}
+
+void CIRGenFunction::emitAutoVarCleanups(const clang::VarDecl &d) {
+  // Check the type for a cleanup.
+  if (QualType::DestructionKind dtorKind = d.needsDestruction(getContext()))
+    cgm.errorNYI(d.getSourceRange(), "emitAutoVarCleanups: type cleanup");
+
+  assert(!cir::MissingFeatures::opAllocaPreciseLifetime());
+
+  // Handle the cleanup attribute.
+  if (d.hasAttr<CleanupAttr>())
+    cgm.errorNYI(d.getSourceRange(), "emitAutoVarCleanups: CleanupAttr");
+}
+
+
+/// 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) {
+  emitAutoVarAlloca(d);
+  emitAutoVarInit(d);
+  emitAutoVarCleanups(d);
 }
 
 void CIRGenFunction::emitVarDecl(const VarDecl &d) {
-  if (d.hasExternalStorage()) {
-    // Don't emit it now, allow it to be emitted lazily on its first use.
+  // If the declaration has external storage, don't emit it now, allow it to be
+  // emitted lazily on its first use.
+  if (d.hasExternalStorage())
     return;
-  }
 
   if (d.getStorageDuration() != SD_Automatic)
     cgm.errorNYI(d.getSourceRange(), "emitVarDecl automatic storage duration");

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

@@ -13,6 +13,7 @@
 #include "Address.h"
 #include "CIRGenFunction.h"
 #include "CIRGenValue.h"
+#include "mlir/IR/BuiltinAttributes.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
@@ -33,11 +34,11 @@ mlir::Value CIRGenFunction::emitLoadOfScalar(LValue lvalue,
   Address addr = lvalue.getAddress();
   mlir::Type eltTy = addr.getElementType();
 
-  auto ptr = addr.getPointer();
+  mlir::Value ptr = addr.getPointer();
   if (mlir::isa<cir::VoidType>(eltTy))
     cgm.errorNYI(loc, "emitLoadOfScalar: void type");
 
-  auto loadOp = builder.CIRBaseBuilderTy::createLoad(getLoc(loc), ptr,
+  mlir::Value loadOp = builder.CIRBaseBuilderTy::createLoad(getLoc(loc), ptr,
                                                      false /*isVolatile*/);
 
   return loadOp;
@@ -54,6 +55,7 @@ RValue CIRGenFunction::emitLoadOfLValue(LValue lv, SourceLocation loc) {
     return RValue::get(emitLoadOfScalar(lv, loc));
 
   cgm.errorNYI(loc, "emitLoadOfLValue");
+  return RValue::get(nullptr);
 }
 
 LValue CIRGenFunction::emitDeclRefLValue(const DeclRefExpr *e) {
@@ -92,37 +94,35 @@ LValue CIRGenFunction::emitDeclRefLValue(const DeclRefExpr *e) {
   }
 
   cgm.errorNYI(e->getSourceRange(), "emitDeclRefLValue: unhandled decl type");
+  return LValue();
 }
 
 mlir::Value CIRGenFunction::emitAlloca(StringRef name, mlir::Type ty,
-                                       mlir::Location loc, CharUnits alignment,
-                                       mlir::Value arraySize) {
+                                       mlir::Location loc, CharUnits alignment) {
   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.
+  // CIR uses its own alloca address space rather than follow the target data
+  // layout like original CodeGen. The data layout awareness should be done in
+  // the lowering pass instead.
   assert(!cir::MissingFeatures::addressSpace());
-  auto localVarPtrTy = builder.getPointerTo(ty);
-  auto alignIntAttr = cgm.getSize(alignment);
+  cir::PointerType localVarPtrTy = builder.getPointerTo(ty);
+  mlir::IntegerAttr alignIntAttr = cgm.getSize(alignment);
 
   mlir::Value addr;
   {
     mlir::OpBuilder::InsertionGuard guard(builder);
     builder.restoreInsertionPoint(builder.getBestAllocaInsertPoint(entryBlock));
     addr = builder.createAlloca(loc, /*addr type*/ localVarPtrTy,
-                                /*var type*/ ty, name, alignIntAttr, arraySize);
+                                /*var type*/ ty, name, alignIntAttr);
     assert(!cir::MissingFeatures::opAllocaVarDeclContext());
   }
   return addr;
 }
 
-/// This creates an alloca and inserts it into the entry block if \p ArraySize
-/// is nullptr, otherwise inserts it at the current insertion point of the
+/// This creates an alloca and inserts it  at the current insertion point of the
 /// builder.
 Address CIRGenFunction::createTempAlloca(mlir::Type ty, CharUnits align,
-                                         mlir::Location loc, const Twine &name,
-                                         mlir::Value arraySize) {
-  mlir::Value alloca = emitAlloca(name.str(), ty, loc, align, arraySize);
+                                         mlir::Location loc, const Twine &name) {
+  mlir::Value alloca = emitAlloca(name.str(), ty, loc, align);
   return Address(alloca, ty, align);
 }

clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp

@@ -63,7 +63,7 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
 
   // l-values
   mlir::Value VisitDeclRefExpr(DeclRefExpr *e) {
-    // TODO: Handle constant emission
+    assert(!cir::MissingFeatures::tryEmitAsConstant());
     return emitLoadOfLValue(e);
   }
 
@@ -91,8 +91,8 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
                                    QualType dstType, SourceLocation loc) {
     // No sort of type conversion is implemented yet, but the path for implicit
     // paths goes through here even if the type isn't being changed.
-    srcType = cgf.getContext().getCanonicalType(srcType);
-    dstType = cgf.getContext().getCanonicalType(dstType);
+    srcType = srcType.getCanonicalType();
+    dstType = dstType.getCanonicalType();
     if (srcType == dstType)
       return src;
 
@@ -120,9 +120,6 @@ mlir::Value ScalarExprEmitter::VisitCastExpr(CastExpr *ce) {
   QualType destTy = ce->getType();
   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
-  // in the same order as in the CastKind enum.
   switch (kind) {
   case CK_LValueToRValue:
     assert(cgf.getContext().hasSameUnqualifiedType(e->getType(), destTy));

clang/lib/CIR/CodeGen/CIRGenFunction.cpp

@@ -13,6 +13,7 @@
 #include "CIRGenFunction.h"
 
 #include "clang/AST/GlobalDecl.h"
+#include "clang/CIR/MissingFeatures.h"
 
 #include <cassert>
 
@@ -131,6 +132,21 @@ mlir::Location CIRGenFunction::getLoc(mlir::Location lhs, mlir::Location rhs) {
   return mlir::FusedLoc::get(locs, metadata, &getMLIRContext());
 }
 
+mlir::LogicalResult CIRGenFunction::declare(Address addr, const Decl *var,
+                                            QualType ty, mlir::Location loc,
+                                            CharUnits alignment) {
+  const auto *namedVar = dyn_cast_or_null<NamedDecl>(var);
+  assert(namedVar && "Needs a named decl");
+  assert(!cir::MissingFeatures::cgfSymbolTable());
+
+  mlir::Value addrVal = addr.getPointer();
+  auto allocaOp = cast<cir::AllocaOp>(addrVal.getDefiningOp());
+  if (ty->isReferenceType() || ty.isConstQualified())
+    allocaOp.setConstantAttr(mlir::UnitAttr::get(&getMLIRContext()));
+
+  return mlir::success();
+}
+
 void CIRGenFunction::startFunction(GlobalDecl gd, QualType returnType,
                                    cir::FuncOp fn, cir::FuncType funcType,
                                    SourceLocation loc,