[Calyx] Implement combinational groups #1736

mortbopet · 2021-09-06T20:13:22Z

Adds a new kind of groups CombOpGroup for representing combinational groups. Fixes #1735.

Combinational groups have no terminators.
group_go and group_done are implicitly not allowed in comb_groups since they only accept GroupOps as a parent.
A new interface GroupOpInterface has been implemented to allow for sharing most of the logic for validation/emission of both GroupOp and CombGroupOp

Some of the tests in errors.mlir were failing - i suspect this is due to changes in the ordering of how validators are applied. Modified where necessary to isolate the error that is tested for.

Adds a new kind of groups `CombOpGroup` for representing combinational groups. Fixes llvm#1735. - Combinational groups have no terminators. - `group_go` and `group_done` are implicitly not allowed in comb_groups since they only accept `GroupOp`s as a parent. - A new interface `GroupOpInterface` has been implemented to allow for sharing most of the logic for validation/emission of both `GroupOp` and `CombGroupOp` Some of the tests in errors.mlir were failing - i suspect this is due to changes in the ordering of how validators are applied. Modified where necessary to isolate the error that is tested for.

test/Dialect/Calyx/emit.mlir

rachitnigam · 2021-09-07T02:28:30Z

test/Dialect/Calyx/emit.mlir

@@ -102,6 +106,12 @@ calyx.program {
              calyx.if %c1.in with @Group2 {
                calyx.enable @Group2
              }
+              calyx.if %c1.in {
+                calyx.enable @Group2


Another restriction on comb group that I might have not mentioned: They cannot appear inside normal control operators like seq or par. The places where comb group and group may appear are disjoint.

rachitnigam

This looks super good! Two suggestions:

Verifier check to ensure that comb group don't show up in normal control operators such as seq.
Change the emitter to print comb group for combinational groups.

cgyurgyik

Opened #1738 before seeing Rachit's comment above. Agreed. Also, we should verify that combinational group does not have a GroupGoOp or GroupDoneOp. Other than that, a few nits.

Edit: opened #1739 as well. We eventually want to verify the group is indeed only comprised of combinational cells.

cgyurgyik · 2021-09-07T03:04:39Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

@@ -522,7 +523,16 @@ void Emitter::emitGroup(GroupOp group) {
          });
    }
  };
-  emitCalyxSection("group", emitGroupBody, group.sym_name());
+  auto prefix =


nit (feel free to ignore): Is TypeSwitch necessary here? Seems to produce more code than necessary.

Not strictly necessary; in general i like having an error if/when a new class is added to an interface but support is yet to be added. In the given case, however, we're probably not expecting a bunch of new group ops to be added - I can remove it if you prefer.

Yeah that was my thought process. A one liner for this may be

// And even simpler if you factor out "group" StringRef prefix = isa<CombGroupOp>(op) ? "comb group" : "group";

However, do as you please. This is mostly just me nitpicking.

cgyurgyik · 2021-09-07T03:07:05Z

lib/Dialect/Calyx/CalyxOps.cpp

+  if (whileOp.body().front().empty())
+    return whileOp.emitError() << "empty body region.";
+
+  auto optGroupName = whileOp.groupName();


Nit: Let's try to to avoid auto unless it is extremely long (e.g. iterators) or extremely obvious (e.g. cast<MyType>()). It makes the code more friendly for new Calyx users and future contributors. Let me know if you disagree.

I've come to pretty much using auto for everything but built-in types, which (to me) also seems to be the general approach in much of the MLIR codebase. However, in the given case, i agree that Optional<StringRef> here would aid in being explicit about what is going on.

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Again, I find that auto can obfuscate code. Something that is obvious to us may not be obvious to first-time readers. I don't imagine the type of say, groupName, will change any time soon, but perhaps I'm wrong.

cgyurgyik · 2021-09-07T03:16:21Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

@@ -297,7 +297,8 @@ struct Emitter {
          .template Case<IfOp, WhileOp>([&](auto op) {
            indent() << (isa<IfOp>(op) ? "if " : "while ");
            emitValue(op.cond(), /*isIndented=*/false);
-            os << " with " << op.groupName();
+            if (auto groupName = op.groupName(); groupName.hasValue())


Nice use of if statement with initializer :)

mortbopet · 2021-09-07T06:58:45Z

@cgyurgyik @rachitnigam Thank you for the reviews and comments; i'll go ahead an implement the things which are missing.

Also, we should verify that combinational group does not have a GroupGoOp or GroupDoneOp.

I initially had an explicit check for this in a verifier for CombOpGroup, however decided to leave it out since GroupGoOp and GroupDoneOp only allow a GroupOp as parent:

circt/include/circt/Dialect/Calyx/Calyx.td

Lines 73 to 75 in 438ffeb

    
           class CalyxGroupPort<string mnemonic, list<OpTrait> traits = []> : 
        
             CalyxOp<mnemonic, !listconcat(traits, [ 
        
               HasParent<"GroupOp">

So as i see it, adding an explicit check is just trying to do the job of the built-in checks. The only issue is that it is a bit implicit...

mortbopet · 2021-09-07T07:42:49Z

@rachitnigam

Verifier check to ensure that comb group don't show up in normal control operators such as seq.

This boils down to comb groups not being allowed with calyx.enable, right?

rachitnigam · 2021-09-07T09:05:31Z

Yup! Enable cannot contain comb groups.

mortbopet · 2021-09-07T09:24:13Z

This should now be handled in 0af4cb6

rachitnigam · 2021-09-07T09:57:03Z

Awesome! Let me know when this PR is ready to merge and I can coordinate the merge with calyxir/calyx#635

rachitnigam · 2021-09-07T09:57:48Z

Worth linking this issue if this PR closes it: #1738

cgyurgyik

@cgyurgyik @rachitnigam Thank you for the reviews and comments; i'll go ahead an implement the things which are missing.

Also, we should verify that combinational group does not have a GroupGoOp or GroupDoneOp.

I initially had an explicit check for this in a verifier for CombOpGroup, however decided to leave it out since GroupGoOp and GroupDoneOp only allow a GroupOp as parent:

circt/include/circt/Dialect/Calyx/Calyx.td

Lines 73 to 75 in 438ffeb

class CalyxGroupPort<string mnemonic, list<OpTrait> traits = []> :

CalyxOp<mnemonic, !listconcat(traits, [

HasParent<"GroupOp">

So as i see it, adding an explicit check is just trying to do the job of the built-in checks. The only issue is that it is a bit implicit...

Ah yeah you're right! This should be good enough.

cgyurgyik · 2021-09-07T15:41:06Z

lib/Dialect/Calyx/Export/CalyxEmitter.cpp

@@ -522,7 +523,16 @@ void Emitter::emitGroup(GroupOp group) {
          });
    }
  };
-  emitCalyxSection("group", emitGroupBody, group.sym_name());
+  auto prefix =


Yeah that was my thought process. A one liner for this may be

// And even simpler if you factor out "group" StringRef prefix = isa<CombGroupOp>(op) ? "comb group" : "group";

However, do as you please. This is mostly just me nitpicking.

cgyurgyik · 2021-09-07T15:43:36Z

lib/Dialect/Calyx/CalyxOps.cpp

+  if (whileOp.body().front().empty())
+    return whileOp.emitError() << "empty body region.";
+
+  auto optGroupName = whileOp.groupName();


https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Again, I find that auto can obfuscate code. Something that is obvious to us may not be obvious to first-time readers. I don't imagine the type of say, groupName, will change any time soon, but perhaps I'm wrong.

cgyurgyik · 2021-09-07T15:46:26Z

lib/Dialect/Calyx/CalyxOps.cpp

-           << "with group: " << groupName << ", which does not exist.";
+           << "with group '" << groupName << "', which does not exist.";
+
+  if (isa<CombGroupOp>(groupOp))


mortbopet requested review from mikeurbach and cgyurgyik September 6, 2021 20:13

remove some whitespace

0e12945

rachitnigam reviewed Sep 7, 2021

View reviewed changes

test/Dialect/Calyx/emit.mlir Outdated Show resolved Hide resolved

rachitnigam reviewed Sep 7, 2021

View reviewed changes

cgyurgyik mentioned this pull request Sep 7, 2021

[Calyx] Combinational and non-combinational groups are disjoint. #1738

Closed

cgyurgyik reviewed Sep 7, 2021

View reviewed changes

mortbopet added 2 commits September 7, 2021 08:43

Fix 'comb_group' => 'comb group' in emitter

fb2c163

Be explicit about optional values

8fe3f32

Disallow CombGroupOp in calyx.enable

0af4cb6

Missing newline

b7b9416

cgyurgyik approved these changes Sep 7, 2021

View reviewed changes

cgyurgyik reviewed Sep 7, 2021

View reviewed changes

Some nits

e323024

mortbopet merged commit b7a4934 into llvm:main Sep 7, 2021

mortbopet deleted the calyx/comb_groups branch October 26, 2021 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Calyx] Implement combinational groups #1736

[Calyx] Implement combinational groups #1736

mortbopet commented Sep 6, 2021

rachitnigam Sep 7, 2021

rachitnigam left a comment

cgyurgyik left a comment •

edited

Loading

cgyurgyik Sep 7, 2021

mortbopet Sep 7, 2021

cgyurgyik Sep 7, 2021 •

edited

Loading

cgyurgyik Sep 7, 2021

mortbopet Sep 7, 2021

cgyurgyik Sep 7, 2021

cgyurgyik Sep 7, 2021

mortbopet commented Sep 7, 2021

mortbopet commented Sep 7, 2021

rachitnigam commented Sep 7, 2021

mortbopet commented Sep 7, 2021

rachitnigam commented Sep 7, 2021

rachitnigam commented Sep 7, 2021

cgyurgyik left a comment

cgyurgyik Sep 7, 2021 •

edited

Loading

cgyurgyik Sep 7, 2021

cgyurgyik Sep 7, 2021

	class CalyxGroupPort<string mnemonic, list<OpTrait> traits = []> :
	CalyxOp<mnemonic, !listconcat(traits, [
	HasParent<"GroupOp">

[Calyx] Implement combinational groups #1736

[Calyx] Implement combinational groups #1736

Conversation

mortbopet commented Sep 6, 2021

Choose a reason for hiding this comment

rachitnigam left a comment

Choose a reason for hiding this comment

cgyurgyik 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

cgyurgyik Sep 7, 2021 • 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

Choose a reason for hiding this comment

mortbopet commented Sep 7, 2021

mortbopet commented Sep 7, 2021

rachitnigam commented Sep 7, 2021

mortbopet commented Sep 7, 2021

rachitnigam commented Sep 7, 2021

rachitnigam commented Sep 7, 2021

cgyurgyik left a comment

Choose a reason for hiding this comment

cgyurgyik Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgyurgyik left a comment •

edited

Loading

cgyurgyik Sep 7, 2021 •

edited

Loading

cgyurgyik Sep 7, 2021 •

edited

Loading