Address review feedback

Author: andykaylor

clang/include/clang/CIR/Dialect/Passes.td

@@ -14,9 +14,9 @@ include "mlir/Pass/PassBase.td"
 def CIRFlattenCFG : Pass<"cir-flatten-cfg"> {
   let summary = "Produces flatten CFG";
   let description = [{
-    This pass transforms CIR and inline all the nested regions. Thus,
-    the next post condtions are met after the pass applied:
-    - there is not any nested region in a function body
+    This pass transforms CIR by inlining all the nested regions. Thus,
+    the following condtions are true after the pass applied:
+    - there are no nested regions in any function body
     - all the blocks in a function belong to the parent region
     In other words, this pass removes such CIR operations like IfOp, LoopOp,
     ScopeOp and etc. and produces a flat CIR.

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

@@ -44,7 +44,10 @@ class CIRScopeOpFlattening : public mlir::OpRewritePattern<cir::ScopeOp> {
 
     // Empty scope: just remove it.
     // TODO: Remove this logic once CIR uses MLIR infrastructure to remove
-    // trivially dead operations
+    // trivially dead operations. MLIR canonicalizer is too aggressive and we
+    // need to either (a) make sure all our ops model all side-effects and/or
+    // (b) have more options in the canonicalizer in MLIR to temper
+    // aggressiveness level.
     if (scopeOp.isEmpty()) {
       rewriter.eraseOp(scopeOp);
       return mlir::success();

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

@@ -33,6 +33,7 @@
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TimeProfiler.h"
 
 using namespace cir;
@@ -138,6 +139,7 @@ mlir::LLVM::Linkage convertLinkage(cir::GlobalLinkageKind linkage) {
   case CIR::WeakODRLinkage:
     return LLVM::WeakODR;
   };
+  llvm_unreachable("Unknown CIR linkage type");
 }
 
 class CIRAttrToValue {
@@ -606,10 +608,15 @@ static void prepareTypeConverter(mlir::LLVMTypeConverter &converter,
   });
 }
 
-// The unreachable code is not lowered by applyPartialConversion function
-// since it traverses blocks in the dominance order. At the same time we
-// do need to lower such code - otherwise verification errors occur.
-// For instance, the next CIR code:
+// The applyPartialConversion function traverses blocks in the dominance order,
+// so it does not lower and operations that are not reachachable from the
+// operations passed in as arguments. Since we do need to lower such code in
+// order to avoid verification errors occur, we cannot just pass the module op
+// to applyPartialConversion. We must build a set of unreachable ops and
+// explicitly add them, along with the module, to the vector we pass to
+// applyPartialConversion.
+//
+// For instance, this CIR code:
 //
 //    cir.func @foo(%arg0: !s32i) -> !s32i {
 //      %4 = cir.cast(int_to_bool, %arg0 : !s32i), !cir.bool
@@ -620,18 +627,18 @@ static void prepareTypeConverter(mlir::LLVMTypeConverter &converter,
 //        %5 = cir.const #cir.int<0> : !s32i
 //       cir.return %5 : !s32i
 //      }
-//     cir.return %arg0 : !s32i
+//      cir.return %arg0 : !s32i
 //    }
 //
 // contains an unreachable return operation (the last one). After the flattening
-// pass it will be placed into the unreachable block. And the possible error
+// pass it will be placed into the unreachable block. The possible error
 // after the lowering pass is: error: 'cir.return' op expects parent op to be
 // one of 'cir.func, cir.scope, cir.if ... The reason that this operation was
 // not lowered and the new parent is llvm.func.
 //
-// In the future we may want to get rid of this function and use DCE pass or
-// something similar. But now we need to guarantee the absence of the dialect
-// verification errors.
+// In the future we may want to get rid of this function and use a DCE pass or
+// something similar. But for now we need to guarantee the absence of the
+// dialect verification errors.
 static void collectUnreachable(mlir::Operation *parent,
                                llvm::SmallVector<mlir::Operation *> &ops) {
 

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVMIR.cpp

@@ -33,16 +33,7 @@ class CIRDialectLLVMIRTranslationInterface
     : public mlir::LLVMTranslationDialectInterface {
 public:
   using LLVMTranslationDialectInterface::LLVMTranslationDialectInterface;
-#if 0
-  /// Any named attribute in the CIR dialect, i.e, with name started with
-  /// "cir.", will be handled here.
-  mlir::LogicalResult amendOperation(
-      mlir::Operation *op, llvm::ArrayRef<llvm::Instruction *> instructions,
-      mlir::NamedAttribute attribute,
-      mlir::LLVM::ModuleTranslation &moduleTranslation) const override {
-    return mlir::success();
-  }
-#endif
+
   /// Translates the given operation to LLVM IR using the provided IR builder
   /// and saving the state in `moduleTranslation`.
   mlir::LogicalResult convertOperation(