[Moore] A new pass to delete local temporary variables. #6990

hailongSun2000 · 2024-05-06T10:41:14Z

Hey, @fabianschuiki! Please review this at your convenience! Thanks in advance!
This PR is about deleting local variables.
For example:

int x, y, z;
always_comb begin
  automatic int a;
  a = x + 1;
  y = a;
  a = a + 1;
  z = a;
end

To

int x, y, z;
y = x + 1;
z = (x + 1) + 1;

Anything about code details could be pointed out.

hailongSun2000 · 2024-05-06T10:49:41Z

The further IR details:
From

module {
  moore.module @Foo {
    %x = moore.variable : !moore.int
    %y = moore.variable : !moore.int
    %z = moore.variable : !moore.int
    moore.procedure always_comb {
      %a = moore.variable : !moore.int
      %0 = moore.constant 1 : !moore.int
      %1 = moore.add %x, %0 : !moore.int
      moore.blocking_assign %a, %1 : !moore.int
      moore.blocking_assign %y, %a : !moore.int
      %2 = moore.constant 1 : !moore.int
      %3 = moore.add %a, %2 : !moore.int
      moore.blocking_assign %a, %3 : !moore.int
      moore.blocking_assign %z, %a : !moore.int
    }
  }
}

To

moore.procedure always_comb {
  %0 = moore.constant 1 : !moore.int
  %1 = moore.add %x, %0 : !moore.int
  moore.blocking_assign %y, %1 : !moore.int
  %2 = moore.constant 1 : !moore.int
  %3 = moore.add %1, %2 : !moore.int
  moore.blocking_assign %z, %3 : !moore.int
}

or
From

module Foo;
  int x, y, z;
  bit cond;
  always_latch begin
    automatic int a;
    if(!cond) begin
      a = x + 10;
      y = a;
      if(cond) begin
        a = a + 3;
        z = a;
      end
      else begin
        z = y + a;
      end
    end
  end
endmodule

moore.procedure always_latch {
      %a = moore.variable : !moore.int
      %0 = moore.not %cond : !moore.bit
      %1 = moore.conversion %0 : !moore.bit -> i1
      scf.if %1 {
        %2 = moore.constant 10 : !moore.int
        %3 = moore.add %x, %2 : !moore.int
        moore.blocking_assign %a, %3 : !moore.int
        moore.blocking_assign %y, %a : !moore.int
        %4 = moore.conversion %cond : !moore.bit -> i1
        scf.if %4 {
          %5 = moore.constant 3 : !moore.int
          %6 = moore.add %a, %5 : !moore.int
          moore.blocking_assign %a, %6 : !moore.int
          moore.blocking_assign %z, %a : !moore.int
        } else {
          %5 = moore.add %y, %a : !moore.int
          moore.blocking_assign %z, %5 : !moore.int
        }
      }
    }

To

y = x + 10;
if(cond){
z = x + 10 + 3;
}
else{
z = x + 10 + x + 10
}

moore.procedure always_latch {
  %0 = moore.not %cond : !moore.bit
  %1 = moore.conversion %0 : !moore.bit -> i1
  scf.if %1 {
    %2 = moore.constant 10 : !moore.int
    %3 = moore.add %x, %2 : !moore.int
    moore.blocking_assign %y, %3 : !moore.int
    %4 = moore.conversion %cond : !moore.bit -> i1
    scf.if %4 {
      %5 = moore.constant 3 : !moore.int
      %6 = moore.add %3, %5 : !moore.int
      moore.blocking_assign %z, %6 : !moore.int
    } else {
      %5 = moore.add %y, %3 : !moore.int
      moore.blocking_assign %z, %5 : !moore.int
    }
  }
}

The above-mentioned are just one step before generating the Core IR.

hailongSun2000 · 2024-05-06T11:04:33Z

All Build and Tests are passing on my personal computer. It's so strange!

lib/Dialect/Moore/Transforms/DeleteLocalVar.cpp

include/circt/Dialect/Moore/MoorePasses.td

lib/Dialect/Moore/Transforms/DeleteLocalVar.cpp

include/circt/Dialect/Moore/MoorePasses.td

fabianschuiki

Great to see this take shape!

It would be nice to have a bunch of tests that make sure all the different code paths through the pass' code work as expected. And that also make sure that things which the pass does not yet support (for example, myStruct.myField[42] = x) either produce an error message or are left in their original state.

lib/Dialect/Moore/Transforms/DeleteLocalVar.cpp

lib/Conversion/MooreToCore/MooreToCore.cpp

include/circt/Dialect/Moore/MoorePasses.h

hailongSun2000 · 2024-05-08T01:52:37Z

Great to see this take shape!

It would be nice to have a bunch of tests that make sure all the different code paths through the pass' code work as expected. And that also make sure that things which the pass does not yet support (for example, myStruct.myField[42] = x) either produce an error message or are left in their original state.

Yeah, I'll test more cases to ensure its correction.

tools/circt-verilog/circt-verilog.cpp

fabianschuiki · 2024-05-09T16:11:37Z

tools/circt-verilog/circt-verilog.cpp

+/// Parse specified files that had been translated into Moore dialect IR. After
+/// that simplify these files like deleting local variables, and then emit the
+/// resulting Moore dialect IR .
+static LogicalResult populateMooreTransforms(MLIRContext &context,
+                                             ModuleOp module) {
+  PassManager pm(&context);
+  pm.addNestedPass<moore::SVModuleOp>(circt::moore::createDeleteLocalVarPass());
+  // TODO: like dedup pass.
+
+  if (failed(pm.run(module)))
+    return failure();
+
+  return success();
+}
+
+/// Convert Moore dialect IR into core dialect IR
+static LogicalResult populateLowMooreToCore(MLIRContext &context,
+                                            ModuleOp module) {
+  PassManager pm(&context);
+  pm.addPass(createConvertMooreToCorePass());
+
+  if (failed(pm.run(module)))
+    return failure();
+
+  return success();
+}
+


It's great that you have dedicated populate* functions to populate the pipeline 🥳

Instead of creating a pass manager in these functions and running it immediately, the populate functions should accept the pass manager as a function argument and then just populate it with passes (that's why the function name starts with "populate"). For example: https://github.com/llvm/circt/blob/main/lib/Firtool/Firtool.cpp#L26-L49

The firtool tool then calls these populate functions to fill in its own pass manager, and then at the very end calls pm.run to run everything: https://github.com/llvm/circt/blob/main/tools/firtool/firtool.cpp#L377-L481

Thanks for your detailed explanation!
Later, I'll enumerate some examples about DeleteLocalVarPass.

hailongSun2000 · 2024-05-10T09:11:27Z

Great to see this take shape!

It would be nice to have a bunch of tests that make sure all the different code paths through the pass' code work as expected. And that also make sure that things which the pass does not yet support (for example, myStruct.myField[42] = x) either produce an error message or are left in their original state.

I have an example:

module Foo;
  int x, y;
  always_comb begin
    typedef struct packed signed {int a, b;} int64;
    int64 ii;
    ii.a = x + 1;                  // error: unsupported expression: MemberAccess
    ii.b = x;
    y = ii.a;
  end
endmodule

Or

module Foo;
  int x, y;
  always_comb begin
    typedef struct packed signed {int a,b;} int64;
    int64 ii = '{ x+1, x};       // error: unsupported expression: SimpleAssignmentPattern
    y = ii;
  end
endmodule

However, these error messages are thrown by importVerilog because we don't implement them.
So may I handle/verify the local struct int64 after supporting the related struct?

fabianschuiki · 2024-05-10T22:23:59Z

lib/Dialect/Moore/Transforms/DeleteLocalVar.cpp

+          if (valueSymbols.count(destOp)) {
+            if (auto srcValue = valueSymbols.lookup(srcOp))
+              valueSymbols.insert(destOp, srcValue);
+            else
+              valueSymbols.insert(destOp, op.getSrc());
+
+            isNewValue = true;
+          }


I think all you need is an else branch here that emits an error. Something like "cannot assign this expression". This will make sure that the user sees a proper error for anything that the pass does not yet fully support.

If I don't misunderstand, I need to add an else branch. Like if(valueSymbols.count(destOp)){...} else{error:...} . But this will lead to global variables also triggering this error.

fabianschuiki · 2024-05-10T22:27:43Z

By the way, @dtzSiFive has mentioned that MLIR has a mem2reg pass: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Transforms/Mem2Reg.cpp

You might be able to rely on that pass to do the heavy lifting for getting rid of local variables.

hailongSun2000 · 2024-05-11T01:39:21Z

By the way, @dtzSiFive has mentioned that MLIR has a mem2reg pass: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Transforms/Mem2Reg.cpp

You might be able to rely on that pass to do the heavy lifting for getting rid of local variables.

I'll try it again. Originally, I attempted to call createMem2Reg, but it didn't work to delete local variables. Maybe I got the call createMem2Reg wrong.

hailongSun2000 · 2024-05-11T05:47:56Z

Yeah, Mem2Reg doesn't handle moore IR. It just looks like --parse-only.

fabianschuiki · 2024-05-11T06:11:17Z

I think the Mem2Reg pass uses op interfaces to find out what certain ops do and how it has to handle them. You may be able to add those interfaces to the relevant moore ops to make Mem2Reg able to work with them 🙂

cepheus69 · 2024-05-11T06:19:48Z

That means we need new interfaces for specified operations 🤔 So MooreOpInterface is coming ?

fabianschuiki · 2024-05-11T21:27:13Z

I think all you need is to add the PromotableOpInterface, PromotableOpInterface, or other interface that Mem2Reg uses to the list of traits of the corresponding Moore op definition. The InstanceOp already does this for the SymbolUserOpInterface: https://github.com/llvm/circt/blob/main/include/circt/Dialect/Moore/MooreOps.td#L64-L66

As far as I can tell, only the MemRef and LLVMIR dialects in MLIR support mem2reg, or at least have a test for it: https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/MemRef/mem2reg.mlir. But you may be able to look at the op definitions in the MemRef dialect to see which of the Mem2Reg op interfaces you have to implement.

hailongSun2000 · 2024-05-12T00:33:08Z

Yeah, that's right! We don't create a new OpInterface for moore dialect. At least not for now. I was yesterday referred to LLVMIR which also uses the PromotableOpInterface and other Promotable*OpInterface. However, I met the ld.lld error(undefined symbol). Maybe I forgot something to link it in the CMakeLists.

Like the above image, I separately added the MemoryslotInterfaces.h and MemoryslotInterfaces.td to the MooreOp.h and MooreOp.td, and added MLIRMemorySlotInterfaces/MLIRTransforms(just one of them, I forgot 😢 ) into CMakeLists. But I'll solve it tomorrow.

hailongSun2000 requested a review from fabianschuiki May 6, 2024 10:41

hailongSun2000 marked this pull request as draft May 6, 2024 11:04

hailongSun2000 added the Verilog/SystemVerilog Involving a Verilog dialect label May 7, 2024

uenoku reviewed May 7, 2024

View reviewed changes

hailongSun2000 force-pushed the hailong/delete-local-variables-pass branch from 6a09633 to f047ad1 Compare May 7, 2024 09:48

hailongSun2000 commented May 7, 2024

View reviewed changes

fabianschuiki reviewed May 7, 2024

View reviewed changes

lib/Dialect/Moore/Transforms/DeleteLocalVar.cpp Outdated Show resolved Hide resolved

lib/Conversion/MooreToCore/MooreToCore.cpp Outdated Show resolved Hide resolved

include/circt/Dialect/Moore/MoorePasses.h Outdated Show resolved Hide resolved

hailongSun2000 force-pushed the hailong/delete-local-variables-pass branch 3 times, most recently from 01e0f6b to 068a28d Compare May 9, 2024 09:53

fabianschuiki reviewed May 9, 2024

View reviewed changes

[Moore] A new pass to delete local temporary variables.

a336f37

hailongSun2000 force-pushed the hailong/delete-local-variables-pass branch from 068a28d to a336f37 Compare May 10, 2024 03:30

fabianschuiki reviewed May 10, 2024

View reviewed changes

hailongSun2000 added Moore and removed Verilog/SystemVerilog Involving a Verilog dialect labels May 11, 2024

fabianschuiki mentioned this pull request May 22, 2024

[MooreToCore] Lower moore.variable into hw.wire. #7075

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Moore] A new pass to delete local temporary variables. #6990

[Moore] A new pass to delete local temporary variables. #6990

hailongSun2000 commented May 6, 2024

hailongSun2000 commented May 6, 2024 •

edited

hailongSun2000 commented May 6, 2024

fabianschuiki left a comment

hailongSun2000 commented May 8, 2024

fabianschuiki May 9, 2024

hailongSun2000 May 10, 2024 •

edited

hailongSun2000 commented May 10, 2024

fabianschuiki May 10, 2024

hailongSun2000 May 11, 2024

fabianschuiki commented May 10, 2024

hailongSun2000 commented May 11, 2024

hailongSun2000 commented May 11, 2024 •

edited

fabianschuiki commented May 11, 2024 via email •

edited

cepheus69 commented May 11, 2024 •

edited

fabianschuiki commented May 11, 2024

hailongSun2000 commented May 12, 2024 •

edited

[Moore] A new pass to delete local temporary variables. #6990

Are you sure you want to change the base?

[Moore] A new pass to delete local temporary variables. #6990

Conversation

hailongSun2000 commented May 6, 2024

hailongSun2000 commented May 6, 2024 • edited

hailongSun2000 commented May 6, 2024

fabianschuiki left a comment

Choose a reason for hiding this comment

hailongSun2000 commented May 8, 2024

fabianschuiki May 9, 2024

Choose a reason for hiding this comment

hailongSun2000 May 10, 2024 • edited

Choose a reason for hiding this comment

hailongSun2000 commented May 10, 2024

fabianschuiki May 10, 2024

Choose a reason for hiding this comment

hailongSun2000 May 11, 2024

Choose a reason for hiding this comment

fabianschuiki commented May 10, 2024

hailongSun2000 commented May 11, 2024

hailongSun2000 commented May 11, 2024 • edited

fabianschuiki commented May 11, 2024 via email • edited

cepheus69 commented May 11, 2024 • edited

fabianschuiki commented May 11, 2024

hailongSun2000 commented May 12, 2024 • edited

hailongSun2000 commented May 6, 2024 •

edited

hailongSun2000 May 10, 2024 •

edited

hailongSun2000 commented May 11, 2024 •

edited

fabianschuiki commented May 11, 2024 via email •

edited

cepheus69 commented May 11, 2024 •

edited

hailongSun2000 commented May 12, 2024 •

edited