[Moore] Add AssignedVarOp and canonicalization for VariableOp. #7251

hailongSun2000 · 2024-06-28T10:20:32Z

The canonicalization for VariableOp is aimed at merging the "easy use" of the variable and its value(continuous assignment) into a new op called assigned_variable. Don't handle blocking_assign and nonBlocking_assign due to the dominance.
The "easy use" is assigning a value to a variable with the whole bit-width, in other words, exclude the bits slice, concatenation operator, etc.

hailongSun2000 · 2024-06-28T10:21:30Z

Appendix:

/home/phoenix/work/HDLBits/test01.sv:4:10: error: Unsupported drive the same wire 'l' two times now
  assign l = 1'b0;
         ^
module {
  moore.module @Foo() {
    %l = moore.net wire : <l1>
    %l_0 = moore.assigned_variable name "l" %1 : <l1>
    %0 = moore.constant true : i1
    %1 = moore.conversion %0 : !moore.i1 -> !moore.l1
    moore.assign %l, %1 : l1
    %2 = moore.constant false : i1
    %3 = moore.conversion %2 : !moore.i1 -> !moore.l1
    moore.assign %l, %3 : l1
    moore.output
  }
}

uenoku

I think it's reasonable to make SVModuleOp graph region op for progressive lowering into core dialects. However it's a big change for Moore dialect design so would be nice to have consensus on this change.

lib/Dialect/Moore/Transforms/MergeAssignments.cpp

uenoku · 2024-07-01T06:08:27Z

include/circt/Dialect/Moore/MooreOps.td

    `:` type($result)
  }];
 }

+def AssignedVarOp : MooreOp<"assigned_variable", [


Do we need a new op? NetOp seems to have an assignment operand so is that same?

You're totally right! NetOp has an assignment operand. Maybe I think I need to rename it. Like def AssignedDeclOp : MooreOp<"assigned_decl(aration)", [, what do @fabianschuiki @uenoku think? By the way, slang will throw an error, if we drive the same variable multiple times.

Hmmm yeah good question... SV differentiates between nets and variables in quite a few places. My gut feeling is that turning a VariableOp into a NetOp with an assignment isn't going to be correct in general 🤔. The AssignedVariableOp feels like a good step, because it retains the variable-ness but makes it explicit that it is continuously assigned a specific value.

I'm pretty sure we'll be able to write some simplification passes that take always_comb procedures that write to variables through moore.blocking_assign and convert that into a simple moore.assigned_variable for simplicity. Then only the truly annoying uses of variables would remain (the ones interacting with the event queue, have delays, etc.).

hailongSun2000 · 2024-07-01T06:17:04Z

I'll separate the changing of SVModule into a different PR.

lib/Dialect/Moore/Transforms/MergeAssignments.cpp

fabianschuiki · 2024-07-01T23:45:46Z

I'm wondering if this could work as a canonicalization pattern defined on VariableOp 🤔. The pattern could theoretically go through all users of the variable op and if there is a single continuous assignment, the assigned value could be merged into a new AssignedVariableOp. But I'm not sure how idiomatic and scalable it is to scan an entire user list in a canonicalization... @uenoku may have an opinion on that 😏

uenoku · 2024-07-02T06:26:52Z

It makes sense to implement it as canonicalization. We do the same thing for SV so compile time should be OK. However in that case please make sure lowering pass can lower them even without canonicalization.

hailongSun2000 · 2024-07-03T10:20:53Z

Hey, @fabianschuiki @uenoku! I add the canonicalization pattern for moore.variable. But it seems can not keep the original variable, if not call replaceOp() /replaceOpWithNewOp(), the canonicalize() will not take effect.

uenoku · 2024-07-03T16:27:33Z

lib/Dialect/Moore/MooreOps.cpp

+  for (auto *user : op->getUsers())
+    if (isa<ContinuousAssignOp>(user) &&
+        (user->getOperand(0) == op.getResult())) {
+      auto assignVarOp = rewriter.create<AssignedVarOp>(
+          op->getLoc(), op.getType(), op.getName(), user->getOperand(1));
+      rewriter.replaceOp(op, assignVarOp);
+      return success();
+    }
+  return failure();
+}


I think you must ensure that there is no multiple ContinuousAssignOp.

Slang has guaranteed that the variable has only one continuous assignment. For example:

int x; assign x = 32; assign x = 64;

source.sv:4:9: error: cannot have multiple continuous assignments to variable 'x' assign x = 64; ^ source.sv:3:9: note: also assigned here assign x = 32;

I'll leave a comment about this.

I think we shouldn't rely on implicit assumption of slang. We should at least check that property in our verifier.

Ok, I'll add this.

I understood why we need to check whether there are multiple continuous assignments for the same variable. Because the --canonicalize operates on the IR, not from AST to IR, right?

Yes, I think we should able to use Moore dialect not only for slang.

lib/Dialect/Moore/MooreOps.cpp

uenoku · 2024-07-03T16:32:08Z

Can we remove mergeAssignments pass if we implemented it as a canonicalization?

hailongSun2000 · 2024-07-04T02:14:53Z

I have removed the MergeAssignments pass.

uenoku · 2024-07-04T07:05:15Z

lib/Dialect/Moore/MooreOps.cpp

+  llvm::DenseSet<Operation *> variables;
+  Value initial;
+  for (auto *user : op->getUsers())
+    if (isa<ContinuousAssignOp>(user) &&
+        (user->getOperand(0) == op.getResult())) {
+      // Don't canonicalize the multiple continuous assignment to the same
+      // variable.
+      if (variables.contains(op))
+        return failure();
+
+      variables.insert(op);
+      initial = user->getOperand(1);
+    }


I think we don't need variables set.

Suggested change

llvm::DenseSet<Operation *> variables;

Value initial;

for (auto *user : op->getUsers())

if (isa<ContinuousAssignOp>(user) &&

(user->getOperand(0) == op.getResult())) {

// Don't canonicalize the multiple continuous assignment to the same

// variable.

if (variables.contains(op))

return failure();

variables.insert(op);

initial = user->getOperand(1);

}

Value initial;

for (auto *user : op->getUsers())

if (isa<ContinuousAssignOp>(user) &&

(user->getOperand(0) == op.getResult())) {

// Don't canonicalize the multiple continuous assignment to the same

// variable.

if (initial)

return failure();

initial = user->getOperand(1);

}

Nice~ ! 💯

uenoku

One nit, otherwise LGTM!

[Moore] Add the canonicalization pattern for VariableOp.

hailongSun2000 · 2024-07-04T07:24:21Z

Thanks for the code review 😃 .

fabianschuiki · 2024-07-09T17:20:10Z

Very cool! 🥳

hailongSun2000 added the Moore label Jun 28, 2024

hailongSun2000 requested review from fabianschuiki, uenoku and maerhart June 28, 2024 10:20

uenoku reviewed Jul 1, 2024

View reviewed changes

hailongSun2000 force-pushed the hailong/add-assigned-variable-op branch from fd08bf5 to 843f558 Compare July 1, 2024 09:37

fabianschuiki reviewed Jul 1, 2024

View reviewed changes

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

fabianschuiki reviewed Jul 1, 2024

View reviewed changes

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

hailongSun2000 force-pushed the hailong/add-assigned-variable-op branch 2 times, most recently from ef922c6 to ade3855 Compare July 2, 2024 03:23

hailongSun2000 changed the title ~~[Moore] Add AssignedVarOp and a graph region for SVModule.~~ [Moore] Add AssignedVarOp and MergeAssignments pass. Jul 3, 2024

hailongSun2000 force-pushed the hailong/add-assigned-variable-op branch 2 times, most recently from c8c19f6 to edce0c1 Compare July 3, 2024 08:21

uenoku reviewed Jul 3, 2024

View reviewed changes

lib/Dialect/Moore/MooreOps.cpp Outdated Show resolved Hide resolved

hailongSun2000 force-pushed the hailong/add-assigned-variable-op branch from dcbc38f to 1234b63 Compare July 4, 2024 02:14

hailongSun2000 changed the title ~~[Moore] Add AssignedVarOp and MergeAssignments pass.~~ [Moore] Add AssignedVarOp and canonicalization for VariableOp. Jul 4, 2024

hailongSun2000 force-pushed the hailong/add-assigned-variable-op branch from 1234b63 to e89aa9e Compare July 4, 2024 03:16

uenoku reviewed Jul 4, 2024

View reviewed changes

uenoku approved these changes Jul 4, 2024

View reviewed changes

[Moore] Add AssignedVarOp.

1f3374f

[Moore] Add the canonicalization pattern for VariableOp.

hailongSun2000 force-pushed the hailong/add-assigned-variable-op branch from e89aa9e to 1f3374f Compare July 4, 2024 07:20

hailongSun2000 merged commit aa03227 into llvm:main Jul 4, 2024
4 checks passed

hailongSun2000 deleted the hailong/add-assigned-variable-op branch July 10, 2024 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Moore] Add AssignedVarOp and canonicalization for VariableOp. #7251

[Moore] Add AssignedVarOp and canonicalization for VariableOp. #7251

hailongSun2000 commented Jun 28, 2024 •

edited

Loading

hailongSun2000 commented Jun 28, 2024

uenoku left a comment •

edited

Loading

uenoku Jul 1, 2024

hailongSun2000 Jul 1, 2024

fabianschuiki Jul 1, 2024

hailongSun2000 commented Jul 1, 2024

fabianschuiki commented Jul 1, 2024

uenoku commented Jul 2, 2024

hailongSun2000 commented Jul 3, 2024

uenoku Jul 3, 2024

hailongSun2000 Jul 4, 2024 •

edited

Loading

uenoku Jul 4, 2024 •

edited

Loading

hailongSun2000 Jul 4, 2024

hailongSun2000 Jul 4, 2024

uenoku Jul 4, 2024

uenoku commented Jul 3, 2024

hailongSun2000 commented Jul 4, 2024

uenoku Jul 4, 2024

hailongSun2000 Jul 4, 2024

uenoku left a comment

hailongSun2000 commented Jul 4, 2024

fabianschuiki commented Jul 9, 2024

[Moore] Add AssignedVarOp and canonicalization for VariableOp. #7251

[Moore] Add AssignedVarOp and canonicalization for VariableOp. #7251

Conversation

hailongSun2000 commented Jun 28, 2024 • edited Loading

hailongSun2000 commented Jun 28, 2024

uenoku left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hailongSun2000 commented Jul 1, 2024

fabianschuiki commented Jul 1, 2024

uenoku commented Jul 2, 2024

hailongSun2000 commented Jul 3, 2024

Choose a reason for hiding this comment

hailongSun2000 Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

uenoku Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku commented Jul 3, 2024

hailongSun2000 commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku left a comment

Choose a reason for hiding this comment

hailongSun2000 commented Jul 4, 2024

fabianschuiki commented Jul 9, 2024

hailongSun2000 commented Jun 28, 2024 •

edited

Loading

uenoku left a comment •

edited

Loading

hailongSun2000 Jul 4, 2024 •

edited

Loading

uenoku Jul 4, 2024 •

edited

Loading