[CIR] Goto #508

gitoleg · 2024-03-14T08:30:21Z

Please, take a seat, there is a long story ahead.

Intro

The idea is to add a support for goto and this PR is a draft, WIP, a proposal and help seeking.
There is some goto support in the CIR code, but I would say the existing approach won't help us in the real code, where any goto may lead to any place in a function. The existing approach assumes that goto and target label are located in the same lexical scope.

Problem description

Initial desire is just extend current approach and resolve gotos and labels somewhere in the end of function building. But it won't work, since CIR operations contains regions, and it's prohibited to reference a block from another region, e.g. we can't create a branch from IfOp to another or do branching inside a loop. In other words, we just can't apply the same approach here.

Implementation in CIR

Thus, we need operations - cir.goto and cir.label. The latter could be replaced with an attribute for a block, but it's not possible as far I understand. Since labels are unique inside a function - we can use just string attribute here. Thus, for a simple code like:

void bar(int x) {
  if (x > 5)
    goto loop;

  while (x < 10) {    
    loop:
      x++;
  }
}

we get the next CIR code

 cir.func @bar(%arg0: !s32i)  {
    %0 = cir.alloca !s32i, cir.ptr <!s32i>, ["x", init] {alignment = 4 : i64}
    cir.store %arg0, %0 : !s32i, cir.ptr <!s32i>
    cir.scope {
      %1 = cir.load %0 : cir.ptr <!s32i>, !s32i
      %2 = cir.const(#cir.int<5> : !s32i) : !s32i
      %3 = cir.cmp(gt, %1, %2) : !s32i, !s32i
      %4 = cir.cast(int_to_bool, %3 : !s32i), !cir.bool
      cir.if %4 {
        cir.goto <"loop">
      }
    }
    cir.scope {
      cir.while {
        %1 = cir.load %0 : cir.ptr <!s32i>, !s32i
        %2 = cir.const(#cir.int<10> : !s32i) : !s32i
        %3 = cir.cmp(lt, %1, %2) : !s32i, !s32i
        %4 = cir.cast(int_to_bool, %3 : !s32i), !cir.bool
        cir.condition(%4)
      } do {
        cir.label <"loop">
        %1 = cir.load %0 : cir.ptr <!s32i>, !s32i
        %2 = cir.unary(inc, %1) : !s32i, !s32i
        cir.store %2, %0 : !s32i, cir.ptr <!s32i>
        cir.yield
      }
    }
    cir.return
  }
}

Well, there is a downside here as well - we introduce a new terminator, new branch-like operation which one need to take in account while writing some analysis. But anyway. We don't have much choice here.

Well, looks like that's we need to do in CIR, The next part is lowering.

Lowering Implementation

At first glance, we don't need to do a lot here. Lower GotoOp as following: iterate over the blocks and find one which starts with the corresponded label. It could work in the example above. But here arises a bigger problem. Look:

int check(int x) {
  if (x < 5) 
    goto err;
  
  return 0;
 err:
  return -1;
}

Well, I don't provide a CIR code here, but the code under err label is unreachable from the lowering point of view. And we can't just connect blocks and think we've solved the problem. Unreachable - means that they won't be visited during the lowering.

Lowering observations

Here is some thoughts about lowering.
The good thing - and actually it's what make goto possible to implement - is that we don't pass regions into dialect, and inline them instead. Thus, we can refer to any block in a function. Good.
In the same time - take a look, we emit CIR operations while lower to llvm dialect. It's kind of wrong.
So. I think we need a separate pass that will inline regions, i.e. ifOp, ScopeOp, LoopOp ... should be removed from CIR code before lowering and replaced with branches and conditional branches in the same fashion like we do in the lowering.
I didn't remove the said operations from LowerToLLVM in order to make the diff more readable. But actually the plan is that they never appear in the lowering.

This is what RegionInlining pass is intended - it's almost a copy pasta from LowerToLLVM.cpp. So far there are only three operations handled there. Also, I wrote a small function that connects GotoOp and LabelOp there as well- and it's definitely not a good place for that, I need a separate pass for it, Or rename RegionInlining to something else. It doesn't matter right now, though if you have some thought - I'm eager to hear.

Anyway, the point is that we need to have a proper control flow before the lowering. And thus we have it!

Problems and questions

First, I need to understand if the said above is good from your point of view. Maybe there is another possible solution I didn't come to.

Next. There are some problems remains. The biggest one is unreachable blocks that contains CIR code and hence will be rejected by llvm dialect. Unreachable blocks may appear even in the current lowering process when we do regions inlining, blocks splitting.. but it's kind of coincidence (or not?) that theirs operations are replaced by ones from the llvm dialect, while I create exactly CIR operations in the RegionInliningPass.

Thus I create a stub in LowerToLLVM.cpp - that just collect all the unreachable operations and lower them. So may be you know how to do the same better.

Next. SwitchOp is lowered as .. SwitchOp ) and it means we need to invent here something to inline regions as it's done for LoopOp for instance. The only idea I have so far is to create a special service operation that will reflect the one from dialect (the same was done for BrCondOp that never appears in the CIR codegen).

Outro

It's possible to support goto in CIR!

This PR adds `cir.brcond` lowering, which more or less follows the one in LLVM dialect lowering.

This is a suggestion to relax the existing verification even more than we did it in PR llvm#257. Here we also skip verification if a field on the given index is also of incomplete type - and we can not compare it with the result type of the operation. Now the next code fails with type mismatch error: ``` typedef struct Node { struct Node* next; } NodeStru; void foo(NodeStru* a) { a->next = 0; } ``` because the result type is kind of full and the type of field is not (for the reasons discussed in llvm#256). Basically, the problem is in the `GetMemberOp` result type generated as following (via `CIRGenTypes::convertType`) `!cir.ptr<!cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>} #cir.record.decl.ast>>` where the field type at index differs from the record type - compare with `!cir.ptr<!cir.struct<struct "Node" incomplete #cir.record.decl.ast>>` We just slightly relax the previous solution in llvm#257 - and the compilation won't fail in the case of recursive types. Well, if there are some other thoughts?

Just a trivial fix that enables declaration of local structs. Basically, there it's a copy-pasta from the original `CodeGen`, without debug info handling. Co-authored-by: Bruno Cardoso Lopes <bcardosolopes@users.noreply.github.com>

This PR handles globals initializations for c++ code for the case when a global is inited from a function call or another global.

Traditional Clang's codegen generates IDs for anonymous records (e.g. "struct.anon.1") and ensures that they are unique. This patch does the same for CIRGen, which, until now, would just identify any anonymous record as "anon". This will be required to support mutable structs uniquely identified by their names.

Rename `typeName` to just `name`, also use `StringAttr`'s nullability to identify if the record is identified or anonymous. Unnamed structs are also no longer aliased, as they have no unique name to be used in the alias.

This patch adds RTTI support for C++ virtual inheritance. This patch does not include LLVM lowering support.

Lowering Vtable and RTTI globals. Also lowering AddressPoint. based on llvm#259

The `body` attribute is used to identify if a struct type has a body or not. In other words, it separates complete from incomplete structs. For this reason, the `body` attribute was renamed to `incomplete`, matching its keyword in the CIR language.

MLIR's attributes are inherently nullable, so there is no need to use std::optional to represent a nullable attribute. This patch removes std::optional from StructType's ast attribute to simplify its usage.

…ong and untested CastKind::floating path

In the CIR CodeGen function `ScalarExprEmitter::buildScalarCast`, implement conversions from bool to an integral type. This was inadvertently left out in earlier changes. Reorganize the code in the function to be more clear, with better assertion failure messages when encountering an unimplemented construct. This is a partial fix for issue llvm#290

As discussed in llvm#279, we split `CIRGenBuilder` in two parts, which make some of the helpers usable outside of the `CodeGen` part. Basically, I placed casts and binary operations into a separate class, `CIRBaseBuilder`. Later, it can be extended with another helpers. But right now an idea to have a state less builder as a base one.

A silly fix for code like ``` *(char *)0 = 0; ```

) Currently, different specializations of a template will generate distinct types with the same name. To properly differentiate each specialization, this patch includes the template arguments in the name of the type. This will be required to support mutable structs uniquely identified by their names.

Instead of using a single builder for every possible StructType, we now have three builders: identified complete, identified incomplete, and anonymous struct types. This allows us to enforce correctness and to explicitly show the intent when creating a StructType. This patch also adds support for anonymous structs type aliases. When a StructType has no name, it will generate a `ty_anon_<kind>` alias. Conflicts are automatically resolved by MLIR.

There are instances of CIR where anonymous structs are generated as identified structs with an empty name. This patch Adds a verifier for StructType, which ensures structs have a non-empty name. This will be required for properly uniqueing mutable CIR structs.

Essentially, this patch redefines the CIR StructType manually instead of using the autogenerated definition from tablegen. This is the first step to make StructType mutable, as this feature is not yet supported by tablegen. It's mostly a copy of the tablegen definition, with a few notable differences: - A few embellishments are added to make the code more dev-friendly - Addition of a CIRTypesDetails.h file to keep custom storage definitions - The CIR_AnyCIRType constraint is removed, as it is not used and must be defined in C++ to ensure StructType is a part of it. ghstack-source-id: 5f706dc0a61a4a2ed6e2f20ab0937b1a42bfa9cc Pull Request resolved: llvm#302

This allows a named StructType to be mutated after it has been created, if it is identified and incomplete. The motivation for this is to improve the codegen of CIR in certain scenarios where an incomplete type is used and later completed. These usually leave the IR in an inconsistent state, where there are two records types with the same identifier but different definitions (one complete the other incomplete). For example: ```c++ struct Node { Node *next; }; void test(struct Node n) {} ``` Generates: ```mlir !temp_struct = !cir.struct<struct "Node" incomplete> !full_struct = !cir.struct<struct "Node" {!cir.ptr<!temp_struct>}> ``` To generate the `Node` struct type, its members must be created first. However, the `next` member is a recursive reference, so it can only be completed after its parent. This generates a temporary incomplete definition of the `Node` type that remains in the code even after the type to which it refers is completed. As a consequence, accessing the `next` member of a `Node` value fetches the old incomplete version of the type which affects CIR's type-checking capabilities. This patch ensures that, once the parent is fully visited, the `next` member can be completed in place, automatically updating any references to it at a low cost. To represent recursive types, the StructType now is equipped with self-references. These are represented by a `cir.struct` type with just the name of the parent struct that it refers to. The same snippet of code will not generate the following CIR IR: ```mlir !full_struct = !cir.struct<struct "Node" {!cir.ptr<!cir.struct<struct "Node">>}> ``` Summary of the changes made: - Named records are now uniquely identified by their name. An attempt to create a new record with the same will fail. - Anonymous records are uniquely identified by members and other relevant attributes. - StructType has a new `mutate` method that allows it to be mutated after it has been created. Each type can only be mutated if it is identified and incomplete, rendering further changes impossible. - When building a new name StructType, the builder will try to first create, then complete the type, ensuring that: - Inexistent types are created - Existing incomplete types are completed - Existing complete types with matching attributes are reused - Existing complete types with different attributes raise errors - StructType now uses the CyclicParser/Printer guard to avoid infinite recursion and identify when it should print/parse a self-reference. ghstack-source-id: a6d4f650515cbf2d7f6e27d45aae6f768ba44f92 Pull Request resolved: llvm#303

Matrix types are already checked for in `buildScalarConversion`, so they don't need to be checked for again in `buildScalarCast`. Not having to worry about matrix types means the `Element` local variables are no longer necessary. Remove duplicate code by having a variable to store the `CastKind`, and have only one call to `Builder.create<CastOp>`. There are no test changes, because this is refactoring only. There should be no functional changes.

The PR fixes a var initialization with explicit cast.

Add a new entry to enum `CastKind`, `bool_to_float`, since none of the existing enum values adequately covered that conversion. Add code to code gen, CIR validation, LLVM lowering, and the cast test to cover this conversion. Fix ClangIR issue llvm#290

Fix a couple typos in the validation failure messages for scalar casts

github-actions · 2024-03-14T08:33:45Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 9a9729e3ddb7ac6580954cc332ac032c0256b538 46985d2176ae535f42df1c069d9e2fbdcbf8ba9f -- clang/lib/CIR/Dialect/Transforms/RegionInlining.cpp clang/include/clang/CIR/Dialect/Passes.h clang/lib/CIR/CodeGen/CIRGenFunction.cpp clang/lib/CIR/CodeGen/CIRGenFunction.h clang/lib/CIR/CodeGen/CIRGenStmt.cpp clang/lib/CIR/CodeGen/CIRPasses.cpp clang/lib/CIR/Dialect/Transforms/MergeCleanups.cpp clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

View the diff from clang-format here.

diff --git a/clang/lib/CIR/Dialect/Transforms/RegionInlining.cpp b/clang/lib/CIR/Dialect/Transforms/RegionInlining.cpp
index 404261e15d..fdc33703c1 100644
--- a/clang/lib/CIR/Dialect/Transforms/RegionInlining.cpp
+++ b/clang/lib/CIR/Dialect/Transforms/RegionInlining.cpp
@@ -2,15 +2,14 @@
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/IR/PatternMatch.h"
 #include "mlir/Support/LogicalResult.h"
-#include "mlir/Transforms/GreedyPatternRewriteDriver.h"
 #include "mlir/Transforms/DialectConversion.h"
+#include "mlir/Transforms/GreedyPatternRewriteDriver.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 #include "clang/CIR/Dialect/Passes.h"
 
 using namespace mlir;
 using namespace cir;
 
-
 namespace {
 
 /// Lowers operations with the terminator trait that have a single successor.
@@ -35,12 +34,11 @@ void walkRegionSkipping(mlir::Region &region,
   });
 }
 
-
 struct RegionInliningPass : public RegionInliningBase<RegionInliningPass> {
   using RegionInliningBase::RegionInliningBase;
 
   void runOnOperation() override;
-  void process_gotos(SmallVector<GotoOp>& gotos);
+  void process_gotos(SmallVector<GotoOp> &gotos);
 };
 
 struct IfOpRegionsInlining : public OpRewritePattern<IfOp> {
@@ -92,8 +90,7 @@ struct IfOpRegionsInlining : public OpRewritePattern<IfOp> {
     // I believe it can be done in BrCondOp lowering, and we don't need it here.
     rewriter.setInsertionPointToEnd(currentBlock);
     rewriter.create<mlir::cir::BrCondOp>(loc, ifOp.getCondition(),
-                                         thenBeforeBody,
-                                         elseBeforeBody);
+                                         thenBeforeBody, elseBeforeBody);
 
     if (!emptyElse) {
       rewriter.setInsertionPointToEnd(elseAfterBody);
@@ -143,7 +140,7 @@ struct ScopeOpRegionsInlining : public OpRewritePattern<ScopeOp> {
     // Save stack and then branch into the body of the region.
     rewriter.setInsertionPointToEnd(currentBlock);
     // TODO(CIR): stackSaveOp
-    
+
     rewriter.create<mlir::cir::BrOp>(loc, mlir::ValueRange(), beforeBody);
 
     // Replace the scopeop return with a branch that jumps out of the body.
@@ -170,10 +167,9 @@ public:
   using mlir::OpInterfaceRewritePattern<
       mlir::cir::LoopOpInterface>::OpInterfaceRewritePattern;
 
-  inline void
-  lowerConditionOp(mlir::cir::ConditionOp op, mlir::Block *body,
-                   mlir::Block *exit,
-                   mlir::PatternRewriter &rewriter) const {
+  inline void lowerConditionOp(mlir::cir::ConditionOp op, mlir::Block *body,
+                               mlir::Block *exit,
+                               mlir::PatternRewriter &rewriter) const {
     mlir::OpBuilder::InsertionGuard guard(rewriter);
     rewriter.setInsertionPoint(op);
     rewriter.replaceOpWithNewOp<mlir::cir::BrCondOp>(op, op.getCondition(),
@@ -221,7 +217,7 @@ public:
     // if (bodyYield)
     //   lowerTerminator(bodyYield, (step ? step : cond), rewriter);
 
-    for (auto& blk : op.getBody().getBlocks()) {
+    for (auto &blk : op.getBody().getBlocks()) {
       auto bodyYield = dyn_cast<mlir::cir::YieldOp>(blk.getTerminator());
       if (bodyYield)
         lowerTerminator(bodyYield, (step ? step : cond), rewriter);
@@ -243,9 +239,7 @@ public:
   }
 };
 
-
-
-void populateRegionInliningPatterns(RewritePatternSet &patterns) { 
+void populateRegionInliningPatterns(RewritePatternSet &patterns) {
   // clang-format off
   patterns.add<
     IfOpRegionsInlining,
@@ -255,18 +249,18 @@ void populateRegionInliningPatterns(RewritePatternSet &patterns) {
   // clang-format on
 }
 
-// TODO: this is still a draft, not sure it's a good way. Maybe create a separate
-// pass for it
-void RegionInliningPass::process_gotos(SmallVector<GotoOp>& gotos) {
-  for (auto& goTo : gotos) {
+// TODO: this is still a draft, not sure it's a good way. Maybe create a
+// separate pass for it
+void RegionInliningPass::process_gotos(SmallVector<GotoOp> &gotos) {
+  for (auto &goTo : gotos) {
     mlir::ConversionPatternRewriter rewriter(goTo.getContext());
     rewriter.setInsertionPoint(goTo);
     auto func = goTo.getOperation()->getParentOfType<mlir::cir::FuncOp>();
 
-    func.getBody().walk([&](mlir::Block* blk) {
-      for (auto& op : blk->getOperations()) {
+    func.getBody().walk([&](mlir::Block *blk) {
+      for (auto &op : blk->getOperations()) {
         if (auto lab = dyn_cast<mlir::cir::LabelOp>(op)) {
-          if (goTo.getLabel() == lab.getLabel()){
+          if (goTo.getLabel() == lab.getLabel()) {
             lowerTerminator(goTo, blk, rewriter);
             goTo.erase();
             return mlir::WalkResult::interrupt();
@@ -291,7 +285,7 @@ void RegionInliningPass::runOnOperation() {
       ops.push_back(op);
 
     if (auto goTo = dyn_cast<GotoOp>(op))
-        gotos.push_back(goTo);
+      gotos.push_back(goTo);
   });
 
   // Apply patterns.
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index 0543c34593..199a469da5 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -2666,8 +2666,8 @@ class CIRLabelOpLowering
   mlir::LogicalResult
   matchAndRewrite(mlir::cir::LabelOp op, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
-     rewriter.eraseOp(op);
-     return mlir::success();
+    rewriter.eraseOp(op);
+    return mlir::success();
   }
 };
 
@@ -2745,8 +2745,8 @@ void populateCIRToLLVMConversionPatterns(mlir::RewritePatternSet &patterns,
       CIRVectorExtractLowering, CIRVectorCmpOpLowering, CIRVectorSplatLowering,
       CIRVectorTernaryLowering, CIRStackSaveLowering, CIRStackRestoreLowering,
       CIRUnreachableLowering, CIRTrapLowering, CIRInlineAsmOpLowering,
-      CIRSetBitfieldLowering, CIRGetBitfieldLowering,
-      CIRLabelOpLowering>(converter, patterns.getContext());
+      CIRSetBitfieldLowering, CIRGetBitfieldLowering, CIRLabelOpLowering>(
+      converter, patterns.getContext());
 }
 
 namespace {
@@ -2939,31 +2939,30 @@ void ConvertCIRToLLVMPass::runOnOperation() {
   getOperation()->removeAttr("cir.sob");
   getOperation()->removeAttr("cir.lang");
 
-  llvm::SmallVector<mlir::Operation*> ops;
+  llvm::SmallVector<mlir::Operation *> ops;
   ops.push_back(module);
-  llvm::SmallVector<mlir::Block*> unreachable_blocks;
-  module->walk(
-    [&](mlir::Block* blk) {
-      if (blk->hasNoPredecessors() && !blk->isEntryBlock())
-        unreachable_blocks.push_back(blk);
+  llvm::SmallVector<mlir::Block *> unreachable_blocks;
+  module->walk([&](mlir::Block *blk) {
+    if (blk->hasNoPredecessors() && !blk->isEntryBlock())
+      unreachable_blocks.push_back(blk);
   });
 
-  std::set<mlir::Block*> visited;
-  for (auto* root : unreachable_blocks) {
-    
+  std::set<mlir::Block *> visited;
+  for (auto *root : unreachable_blocks) {
+
     // We create a work list for each unreachable block.
     // Thus we traverse operations in some order.
-    std::deque<mlir::Block*> blocks;
+    std::deque<mlir::Block *> blocks;
     blocks.push_back(root);
 
     while (!blocks.empty()) {
-      auto* blk = blocks.back();
+      auto *blk = blocks.back();
       blocks.pop_back();
-      if (visited.count(blk)) 
+      if (visited.count(blk))
         continue;
       visited.emplace(blk);
 
-      for (auto& op : *blk)
+      for (auto &op : *blk)
         ops.push_back(&op);
 
       for (auto it = blk->succ_begin(); it != blk->succ_end(); ++it)
@@ -2971,7 +2970,6 @@ void ConvertCIRToLLVMPass::runOnOperation() {
     }
   }
 
-
   if (failed(applyPartialConversion(ops, target, std::move(patterns))))
     signalPassFailure();

gitoleg · 2024-03-14T08:50:11Z

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

@@ -2924,7 +2966,40 @@ void ConvertCIRToLLVMPass::runOnOperation() {
  getOperation()->removeAttr("cir.sob");
  getOperation()->removeAttr("cir.lang");

-  if (failed(applyPartialConversion(module, target, std::move(patterns))))
+  llvm::SmallVector<mlir::Operation*> ops;


here is the stub I was talking about - and I don't like it. I collect unreachable blocks and then iterate them, discovering successors, add theirs operations and etc.

gitoleg · 2024-03-14T09:00:37Z

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

+
+// TODO: this is still a draft, not sure it's a good way. Maybe create a separate
+// pass for it
+void RegionInliningPass::process_gotos(SmallVector<GotoOp>& gotos) {


here is place where GoTo is replaced by CIR BrOp. Probably it's not a good place - and I can create a separate pass for that after RegionInlining

gitoleg · 2024-03-14T09:02:30Z

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

+            lowerTerminator(op, exit, rewriter);
+        });
+
+    // Lower optional body region yield.


This is the only noticeable change: for some reason YieldOp in the loop body was searched only in the first block, which won't work if there are several in there:

do { ... label: ... } while(...)

bcardosolopes · 2024-03-15T21:58:16Z

Thanks for working on this Oleg, this is long overdue.
I haven't read the actual diff code just yet, let's iterate a bit on the design and converge first.

The existing approach assumes that goto and target label are located in the same lexical scope.

Yep. This is how far I went! I appreciate you are building the real support. For the cir.label and cir.goto, I suggest something like:

cir.func @bar(%arg0: !s32i)
labels = ["loop"]
{
   %0 = cir.alloca !s32i, cir.ptr <!s32i>, ["x", init] {alignment = 4 : i64}

   cir.store %arg0, %0 : !s32i, cir.ptr <!s32i>
   cir.scope {
     %1 = cir.load %0 : cir.ptr <!s32i>, !s32i
     %2 = cir.const(#cir.int<5> : !s32i) : !s32i
     %3 = cir.cmp(gt, %1, %2) : !s32i, !s32i
     %4 = cir.cast(int_to_bool, %3 : !s32i), !cir.bool
     cir.if %4 {
       cir.goto labels[0]
     }
   }
   cir.scope {
     cir.while {
       %1 = cir.load %0 : cir.ptr <!s32i>, !s32i
       %2 = cir.const(#cir.int<10> : !s32i) : !s32i
       %3 = cir.cmp(lt, %1, %2) : !s32i, !s32i
       %4 = cir.cast(int_to_bool, %3 : !s32i), !cir.bool
       cir.condition(%4)
     } do {
       cir.label labels[0]
       %1 = cir.load %0 : cir.ptr <!s32i>, !s32i
       %2 = cir.unary(inc, %1) : !s32i, !s32i
       cir.store %2, %0 : !s32i, cir.ptr <!s32i>
       cir.yield
     }
   }
   cir.return
}

This way we can make the label symbols local to a function, and easily check whether we should run the pass for it. It might require FuncOp to define more symbols than it's name, shouldn't be a problem though? Alternatively, we could have multiple operations cir.label_sym("label") at the function entry block.

cir.label <"loop">

For this one, verifier should make sure it's the first op in a block.

So. I think we need a separate pass that will inline regions, i.e. ifOp, ScopeOp, LoopOp ... should be removed from CIR code before lowering and replaced with branches and conditional branches in the same fashion like we do in the lowering.

Yes! The naive plan I had on my mind was basically the same:

Remove all scope and strucutured control flow before we reach LLVM. The main idea is that when we have a flat hierarchy we can jump back and forth anywhwere.
This pass should happen post LoweringPrepare, but before LLVM lowering.

I didn't remove the said operations from LowerToLLVM in order to make the diff more readable. But actually the plan is that they never appear in the lowering.

Maybe a good first step to build this up would be to start with refactoring out of LowerToLLVM:

Create a new pass "LowerScopes" or "LowerStructuredControlFlow" or something...
Move all block flattening code from LowerToLLVM into LowerScopes.
Update tests until everything passes (LowerToLLVM tests won't probably change, but you'll need new tests for LowerScopes pass).

Once you complete this step, then you create a new "SolveGotos" pass, that will:

Look into functions with labels
Substitute cir.goto to cir.br and erase cir.labels.

How does this sound? Thoughts?

Anyway, the point is that we need to have a proper control flow before the lowering. And thus we have it!

Yes!

gitoleg · 2024-03-18T12:38:43Z

@bcardosolopes
Good to hear we're on the same page!
I suspect the next two PR's needed:

new pass for CIR flattening
goto support itself

speaking about the new pass: I would suggest to embed it into the lowering pipeline, i.e. in the LowerToLLVM - there is a pass manager there. The pros are following:

It will be a good first step, and later we could move this pass in the CIR pipeline
-emit-cir will still produce the same output as it does now, without extra -mlir-print-ir-before=..., so the change will be transparent for users
instead of fixing old tests we could go directly to the work on the new feature. Some of them works for codegen only, e.g. fullexpr.cpp where ScopeOp has a return value. So we need either to rewrite such tests or to handle these missing features as well.

But it's up to you to decide - I'm ready to implement the new pass in the CIR pipeline

UPD: though cir-opt is also needed to be fixed somehow in this case. So may be it's not a good idea from the design point of view.

bcardosolopes · 2024-03-19T00:43:18Z

new pass for CIR flattening

We are actually looking into two new passes:

One for flattening.
One for solving goto's.

goto support itself

Perfect. I'd break into more incremental PRs to make it easier to land/review.

1. Add the skeleton for the new flattening pass. This should a silly test that shows we can test CIR post flattening.
1. move existing flattening from LowerToLLVM to flattening pass.
1. (optional) one PR for each new structured control flow flattening missing. We currently have structured control flow that we don't yet lower to LLVM (cir.try, etc). I'd say you shouldn't need to implement them as a pre-requisite for this work, I'll leave that to your best judgement. But in case you want to do it in order to make goto work with them, this could be a good place to do it.
1. Add new goto/label related ops and CIRGen. This should add tests on top for this operations in pre-flattening CIR.
1. Adds the pass to solve goto's + tests post solving.

speaking about the new pass: I would suggest to embed it into the lowering pipeline, i.e. in the LowerToLLVM - there is a pass manager there. The pros are following:

It will be a good first step, and later we could move this pass in the CIR pipeline

-emit-cir will still produce the same output as it does now, without extra -mlir-print-ir-before=..., so the change will be transparent for users

instead of fixing old tests we could go directly to the work on the new feature. Some of them works for codegen only, e.g. fullexpr.cpp where ScopeOp has a return value. So we need either to rewrite such tests or to handle these missing features as well.

Actually, I don't think current existing tests need to change at all. Not even fullexpr.cpp, given current CIRGen tests are all before LLVM lowering (and pre-flattening) anyways. What am I missing?

I'm not opposed if we start by embeding it into the lowering pipeline, but I'd like that we can test the following scenarios:
a. CIR before flattening.
b. CIR after flattening (CIR before goto solving)
c. CIR after goto solving.

One question that just occured to me is where should emit-cir stop. Should it stop pre-flattening or post goto solving? Should we introduce a new emit-flat-cir which stops after goto solving? Alternatively, we could use emit-cir followed by flags that activate both flattening and goto solving passes. Thoughts?

The suggestion I have for now is to keep emit-cir as is, so we don't need to change existing tests, but I don't have a strong opinion on the subject of the past paragraph.

Also, it sounds to me that -mlir-print-ir-before=... and -mlir-print-ir-after=... are the best mechanisms to achieve fine grained testing capabilities, do you have alternative ideas? Can you use those in face of LowerToLLVM pass manager?

But it's up to you to decide - I'm ready to implement the new pass in the CIR pipeline

UPD: though cir-opt is also needed to be fixed somehow in this case. So may be it's not a good idea from the design point of view.

I'm a bit confused, which case do you mean? cir-opt should work on top of any CIR files, flattened or not, right?

gitoleg · 2024-03-19T14:17:39Z

@bcardosolopes
Let's bring everything together and I will try to explain the exact problems I have so far with respect to the all you said above.

Well, my intention was to add the pass in LowerToLLVM.cpp, namely - somewhere here. But it's not enough - cir-opt -cir-to-llvm doesn't use this code and we need to think how to handle it. Also, it won't let us to emit CIR after each new pass we will add soon.

Next, I could add the pass right after lowering prepare. This would make our life easier - because by doing so we could control the way CIR is emitted exactly as we want - before the pass in question and after. But here is a list of cons:

many tests will need to be fixed - I mean invocation, passing an addition parameter to the header - it's not hard though
future test writers will need to do the same and it may be not that obvious for them what's going on and where are some operations they may want to check
some test are codegen only, i.e. they can't be lowered to llvm dialect because of some checks (take a look at the ScopeOp). And once the code will be moved to the CIR pipeline - these tests will fail. For instance, fullexpr.cpp - it can't be lowered, I double checked) There are not many of them though.
Maybe we will need to introduce more low level representation for SwitchOp (and maybe later for other ops) - because in the modern state it needs a region, as far I understand. And I would keep this low level, internal, lowering-for-only representation hidden from the cir users, at least while the don't need it explicitly
finally, users got used to the fact that -emit-cir looks like it looks now, and from my point of view we should not change it.

So I think I have a solution. We add new passes after the LoweringPrepare but guard them with a bool flag, i.e. - if -emit-cir is enabled, we don't add them. We may add a new option here as well - emit-flat-cir and thus will have everything we need, for instance: -emit-flat-cir -mlir-print-ir-after=cir-structured-cfg.

I'm a bit confused, which case do you mean? cir-opt should work on top of any CIR files, flattened or not, right?

Right!
Probably, we need to change LowerToLLVM.cpp or cir-opt itself and instead of adding the pass with name cir-to-llvm we need to add a pipeline: all these structured-cfg, goto solving and finally the lowering to llvm dialiect itself.

So what do you think? Will it work from your point of view?

bcardosolopes · 2024-03-20T02:18:26Z

Well, my intention was to add the pass in LowerToLLVM.cpp, namely - somewhere here. But it's not enough - cir-opt -cir-to-llvm doesn't use this code and we need to think how to handle it. Also, it won't let us to emit CIR after each new pass we will add soon.

Right, we need to make this a bit more uniform. Perhaps cir-opt and clang should share something along the lines of lowerDirectlyFromCIRToLLVMIR.

many tests will need to be fixed - I mean invocation, passing an addition parameter to the header - it's not hard though. Future test writers will need to do the same and it may be not that obvious for them what's going on and where are some operations they may want to check

Those are good points!

some test are codegen only, i.e. they can't be lowered to llvm dialect because of some checks (take a look at the ScopeOp). And once the code will be moved to the CIR pipeline - these tests will fail. For instance, fullexpr.cpp - it can't be lowered, I double checked) There are not many of them though.

Oh boy, I get it now, thanks for clarifying.

Maybe we will need to introduce more low level representation for SwitchOp (and maybe later for other ops) - because in the modern state it needs a region, as far I understand. And I would keep this low level, internal, lowering-for-only representation hidden from the cir users, at least while the don't need it explicitly

Yep, same is true for future work on exceptions and coroutines.

finally, users got used to the fact that -emit-cir looks like it looks now, and from my point of view we should not change it.

+1

So I think I have a solution. We add new passes after the LoweringPrepare but guard them with a bool flag, i.e. - if -emit-cir is enabled, we don't add them. We may add a new option here as well - emit-flat-cir and thus will have everything we need, for instance: -emit-flat-cir -mlir-print-ir-after=cir-structured-cfg.

LGTM.

Probably, we need to change LowerToLLVM.cpp or cir-opt itself and instead of adding the pass with name cir-to-llvm we need to add a pipeline: all these structured-cfg, goto solving and finally the lowering to llvm dialiect itself.

Indeed, this goes back to my first sentence in this comment: we need to make cir-opt and clang use the same pass setup from CIR down to LLVM.

So what do you think? Will it work from your point of view?

Yes, thanks for pushing this forward.

We start our journey towards `goto` support and this is a first step on this way. There are some discussion in #508 and according to the plan we do the following here: - a new pass called `cir-flatten-cfg` that is a stub now but later will be responsible for the regions inlining. The pass works only if `-emit-flat-cir` is passed in cmd line. Thus, the clang behavior is not changed here from the user's point of view. - The pass will be accomplished with `goto` solver later, so we talk about several passes that are mandatory for the lowering into `llvm` dialect. There are at least two clients of this pass that will be affected: `clang` itself and `cir-opt`, so we need a common point for them: and `populateCIRFlatteningPasses` and `populateCIRToLLVMPasses` guarantee that `CIR` will be in the correct state for all the clients, whatever new passes we will add later.

bcardosolopes · 2024-04-25T22:15:27Z

@gitoleg can we close this one?

gitoleg · 2024-04-26T06:58:19Z

This PR is implemented in the next ones:
#516 #537 #546 #549 #550 #562

We start our journey towards `goto` support and this is a first step on this way. There are some discussion in #508 and according to the plan we do the following here: - a new pass called `cir-flatten-cfg` that is a stub now but later will be responsible for the regions inlining. The pass works only if `-emit-flat-cir` is passed in cmd line. Thus, the clang behavior is not changed here from the user's point of view. - The pass will be accomplished with `goto` solver later, so we talk about several passes that are mandatory for the lowering into `llvm` dialect. There are at least two clients of this pass that will be affected: `clang` itself and `cir-opt`, so we need a common point for them: and `populateCIRFlatteningPasses` and `populateCIRToLLVMPasses` guarantee that `CIR` will be in the correct state for all the clients, whatever new passes we will add later.

We start our journey towards `goto` support and this is a first step on this way. There are some discussion in llvm#508 and according to the plan we do the following here: - a new pass called `cir-flatten-cfg` that is a stub now but later will be responsible for the regions inlining. The pass works only if `-emit-flat-cir` is passed in cmd line. Thus, the clang behavior is not changed here from the user's point of view. - The pass will be accomplished with `goto` solver later, so we talk about several passes that are mandatory for the lowering into `llvm` dialect. There are at least two clients of this pass that will be affected: `clang` itself and `cir-opt`, so we need a common point for them: and `populateCIRFlatteningPasses` and `populateCIRToLLVMPasses` guarantee that `CIR` will be in the correct state for all the clients, whatever new passes we will add later.

We start our journey towards `goto` support and this is a first step on this way. There are some discussion in #508 and according to the plan we do the following here: - a new pass called `cir-flatten-cfg` that is a stub now but later will be responsible for the regions inlining. The pass works only if `-emit-flat-cir` is passed in cmd line. Thus, the clang behavior is not changed here from the user's point of view. - The pass will be accomplished with `goto` solver later, so we talk about several passes that are mandatory for the lowering into `llvm` dialect. There are at least two clients of this pass that will be affected: `clang` itself and `cir-opt`, so we need a common point for them: and `populateCIRFlatteningPasses` and `populateCIRToLLVMPasses` guarantee that `CIR` will be in the correct state for all the clients, whatever new passes we will add later.

We start our journey towards `goto` support and this is a first step on this way. There are some discussion in llvm#508 and according to the plan we do the following here: - a new pass called `cir-flatten-cfg` that is a stub now but later will be responsible for the regions inlining. The pass works only if `-emit-flat-cir` is passed in cmd line. Thus, the clang behavior is not changed here from the user's point of view. - The pass will be accomplished with `goto` solver later, so we talk about several passes that are mandatory for the lowering into `llvm` dialect. There are at least two clients of this pass that will be affected: `clang` itself and `cir-opt`, so we need a common point for them: and `populateCIRFlatteningPasses` and `populateCIRToLLVMPasses` guarantee that `CIR` will be in the correct state for all the clients, whatever new passes we will add later.

We start our journey towards `goto` support and this is a first step on this way. There are some discussion in #508 and according to the plan we do the following here: - a new pass called `cir-flatten-cfg` that is a stub now but later will be responsible for the regions inlining. The pass works only if `-emit-flat-cir` is passed in cmd line. Thus, the clang behavior is not changed here from the user's point of view. - The pass will be accomplished with `goto` solver later, so we talk about several passes that are mandatory for the lowering into `llvm` dialect. There are at least two clients of this pass that will be affected: `clang` itself and `cir-opt`, so we need a common point for them: and `populateCIRFlatteningPasses` and `populateCIRToLLVMPasses` guarantee that `CIR` will be in the correct state for all the clients, whatever new passes we will add later.

lanza and others added 30 commits January 29, 2024 11:36

[CIR][Rebasing] Add header for llvm::join

3f4338a

[CIR][Rebasing] Account for OpenMPIsDevice renaming

c7e3740

[CIR][Rebasing] Adapt a few fns for upstream and adjust some tests

e429fd5

[CIR] Replace an assert with an unreachable

9c427bf

[CIR][Lowering] Add cir.brcond lowering (llvm#278)

52ab8d0

This PR adds `cir.brcond` lowering, which more or less follows the one in LLVM dialect lowering.

[CIR][CodeGen][Bugfix] fixes global vars initialization (llvm#281)

54a62ef

This PR handles globals initializations for c++ code for the case when a global is inited from a function call or another global.

[CIR] Rename StructType "typeName" attribute to "name" (llvm#275)

a751dff

Rename `typeName` to just `name`, also use `StringAttr`'s nullability to identify if the record is identified or anonymous. Unnamed structs are also no longer aliased, as they have no unique name to be used in the alias.

[CIR][Codegen] RTTI support for virtual class inheritence (llvm#259)

53ee1f3

This patch adds RTTI support for C++ virtual inheritance. This patch does not include LLVM lowering support.

[CIR][Lowering] Lower vtable and type info (llvm#264)

d4a20d8

Lowering Vtable and RTTI globals. Also lowering AddressPoint. based on llvm#259

[CIR][Lowering] Fix opaque pointer issue from recent merge

a450eed

[CIR][NFC] Remove std::optional from StructType ast attribute

3be05c3

MLIR's attributes are inherently nullable, so there is no need to use std::optional to represent a nullable attribute. This patch removes std::optional from StructType's ast attribute to simplify its usage.

[CIR][CIRGen][NFC] Fix redudancy while casting integers and remove wr…

be846dd

…ong and untested CastKind::floating path

[CIR][Codegen] Support codegen for FP truncations. (llvm#291)

68ff280

[CIR][CodeGen] Support integer-to-pointer casts. (llvm#298)

51644be

A silly fix for code like ``` *(char *)0 = 0; ```

[CIR] Honor disabling the verifier in lowerDirectlyFromCIRToLLVMIR

8ba9e09

[CIR][CodeGen][Bugfix] fixes explicit cast in initialization (llvm#309)

048199a

The PR fixes a var initialization with explicit cast.

[CIR] Add validation tests for scalar casts (llvm#317)

7f73051

Fix a couple typos in the validation failure messages for scalar casts

gitoleg commented Mar 14, 2024

View reviewed changes

gitoleg added 2 commits March 14, 2024 11:55

adds comment

8daf141

minor fix

26ef3b9

gitoleg commented Mar 14, 2024

View reviewed changes

removes GotoOp lowering

46985d2

gitoleg mentioned this pull request Mar 20, 2024

[CIR] Introduce a flattening pass #516

Merged

lanza force-pushed the main branch 2 times, most recently from ed3955a to 8b7417c Compare March 23, 2024 05:07

lanza force-pushed the main branch from 8b7417c to 330c983 Compare March 30, 2024 04:29

Lancern mentioned this pull request Apr 8, 2024

[CIR][CIRGen] Improve switch support for unrecheable code #528

Closed

lanza force-pushed the main branch from 93715bd to a8b0ecd Compare April 17, 2024 05:15

gitoleg closed this Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR] Goto #508

[CIR] Goto #508

gitoleg commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

gitoleg Mar 14, 2024

gitoleg Mar 14, 2024

gitoleg Mar 14, 2024

bcardosolopes commented Mar 15, 2024

gitoleg commented Mar 18, 2024 •

edited

Loading

bcardosolopes commented Mar 19, 2024

gitoleg commented Mar 19, 2024 •

edited

Loading

bcardosolopes commented Mar 20, 2024

bcardosolopes commented Apr 25, 2024

gitoleg commented Apr 26, 2024 •

edited

Loading

[CIR] Goto #508

[CIR] Goto #508

Conversation

gitoleg commented Mar 14, 2024 • edited Loading

Intro

Problem description

Implementation in CIR

Lowering Implementation

Lowering observations

Problems and questions

Outro

github-actions bot commented Mar 14, 2024 • edited Loading

gitoleg Mar 14, 2024

Choose a reason for hiding this comment

gitoleg Mar 14, 2024

Choose a reason for hiding this comment

gitoleg Mar 14, 2024

Choose a reason for hiding this comment

bcardosolopes commented Mar 15, 2024

gitoleg commented Mar 18, 2024 • edited Loading

bcardosolopes commented Mar 19, 2024

gitoleg commented Mar 19, 2024 • edited Loading

bcardosolopes commented Mar 20, 2024

bcardosolopes commented Apr 25, 2024

gitoleg commented Apr 26, 2024 • edited Loading

gitoleg commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

gitoleg commented Mar 18, 2024 •

edited

Loading

gitoleg commented Mar 19, 2024 •

edited

Loading

gitoleg commented Apr 26, 2024 •

edited

Loading