Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIRRTL] Make IMDCE work for ops w/ regions/blocks #7881

Merged
merged 2 commits into from
Nov 26, 2024
Merged

[FIRRTL] Make IMDCE work for ops w/ regions/blocks #7881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a664331
[FIRRTL] Add memory traits to layerblock
seldridge Nov 25, 2024
d006c2c
[FIRRTL] Make IMDCE work for ops w/ regions/blocks
seldridge Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/circt/Dialect/FIRRTL/FIRRTLStatements.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,8 @@ def LayerBlockOp : FIRRTLOp<
ParentOneOf<[
"firrtl::FModuleOp", "firrtl::LayerBlockOp",
"firrtl::WhenOp", "firrtl::MatchOp"]>,
DeclareOpInterfaceMethods<SymbolUserOpInterface>]
DeclareOpInterfaceMethods<SymbolUserOpInterface>,
RecursiveMemoryEffects, RecursivelySpeculatable]
> {
let summary = "A definition of a layer block";
let description = [{
Expand Down
72 changes: 41 additions & 31 deletions lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "circt/Dialect/HW/InnerSymbolTable.h"
#include "circt/Support/Debug.h"
#include "mlir/IR/ImplicitLocOpBuilder.h"
#include "mlir/IR/Iterators.h"
#include "mlir/IR/Threading.h"
#include "mlir/Interfaces/SideEffectInterfaces.h"
#include "mlir/Pass/Pass.h"
Expand Down Expand Up @@ -240,15 +241,16 @@ void IMDeadCodeElimPass::markBlockExecutable(Block *block) {
if (!executableBlocks.insert(block).second)
return; // Already executable.

auto fmodule = cast<FModuleOp>(block->getParentOp());
if (fmodule.isPublic())
auto fmodule = dyn_cast<FModuleOp>(block->getParentOp());
if (fmodule && fmodule.isPublic())
markAlive(fmodule);

// Mark ports with don't touch as alive.
for (auto blockArg : block->getArguments())
if (hasDontTouch(blockArg)) {
markAlive(blockArg);
markAlive(fmodule);
if (fmodule)
markAlive(fmodule);
Comment on lines +252 to +253
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something seems sketchy to me about this. We don't have many ops other than FModuleOps that have block arguments, but if we did: if a block argument is DontTouch'd, shouldn't we always mark the owning operation as alive, and not if(isa<FModuleOp>(op))? That being said, hasDontTouch(value) first checks to see if the owning op is DontTouchd and then checks for port symbols and annotations iff it is an FModuleOp. It seems a bit weird to me that we say a port is dont touched if the owning module is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you say are the defined semantics of DontTouchAnnotation? 😉

From an annotation perspective, this can only be applied to certain named things (modules, wires, registers, memories, and ports). This annotation has the semantics of, when applied to a module, is equivalent to don't touching all the ports. Given that interpretation, then this is doing the right thing. Or: it's reasonable to expect that this is a module port and not a general block arg as this was never intended to be defined for block args, just for ports.

That said, this annotation has been begging for a better in-IR representation and we've been skirting around this for a while. The best thing to do is to fit this into the FIRRTL spec and then figure out how to represent it here as opposed to trying to generalize don't touch on ports to don't touch on block args (as nobody is asking for that).

}

for (auto &op : *block) {
Expand All @@ -261,8 +263,14 @@ void IMDeadCodeElimPass::markBlockExecutable(Block *block) {
else if (isa<FConnectLike>(op))
// Skip connect op.
continue;
else if (hasUnknownSideEffect(&op))
else if (hasUnknownSideEffect(&op)) {
markUnknownSideEffectOp(&op);
// Recursively mark any blocks contained within these operations as
// executable.
for (auto &region : op.getRegions())
for (auto &block : region.getBlocks())
markBlockExecutable(&block);
}

// TODO: Handle attach etc.
}
Expand Down Expand Up @@ -561,33 +569,35 @@ void IMDeadCodeElimPass::rewriteModuleBody(FModuleOp module) {
std::bind(removeDeadNonLocalAnnotations, -1, std::placeholders::_1));

// Walk the IR bottom-up when deleting operations.
for (auto &op : llvm::make_early_inc_range(llvm::reverse(*body))) {
// Connects to values that we found to be dead can be dropped.
if (auto connect = dyn_cast<FConnectLike>(op)) {
if (isAssumedDead(connect.getDest())) {
LLVM_DEBUG(llvm::dbgs() << "DEAD: " << connect << "\n";);
connect.erase();
++numErasedOps;
}
continue;
}

// Delete dead wires, regs, nodes and alloc/read ops.
if ((isDeclaration(&op) || !hasUnknownSideEffect(&op)) &&
isAssumedDead(&op)) {
LLVM_DEBUG(llvm::dbgs() << "DEAD: " << op << "\n";);
assert(op.use_empty() && "users should be already removed");
op.erase();
++numErasedOps;
continue;
}

// Remove non-sideeffect op using `isOpTriviallyDead`.
if (mlir::isOpTriviallyDead(&op)) {
op.erase();
++numErasedOps;
}
}
module.walk<mlir::WalkOrder::PostOrder, mlir::ReverseIterator>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this work as a preorder walk? If you have a dead op with a region, there is no reason to rewrite the region if the whole thing is going to be deleted anyways. We don't have any ops like this right now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be, however, this would require us to better define whether or not all the ops in the FIRRTL dialect have side effects. Consider the following example:

firrtl.circuit "Foo" {
  firrtl.layer @A bind {
  }
  firrtl.module private @Bar(in %clock: !firrtl.clock, in %a: !firrtl.uint<1>, out %b: !firrtl.probe<uint<1>, @A>, out %c: !firrtl.uint<1>) {
    %0 = firrtl.not %a : (!firrtl.uint<1>) -> !firrtl.uint<1>
    firrtl.matchingconnect %c, %0 : !firrtl.uint<1>
    firrtl.layerblock @A {
      %1 = firrtl.not %a : (!firrtl.uint<1>) -> !firrtl.uint<1>
      %a_not = firrtl.node %1 : !firrtl.uint<1>
      %2 = firrtl.ref.send %a_not : !firrtl.uint<1>
      %3 = firrtl.ref.cast %2 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
      firrtl.ref.define %b, %3 : !firrtl.probe<uint<1>, @A>
    }
  }
  firrtl.module @Foo(in %clock: !firrtl.clock, in %a: !firrtl.uint<1>, out %c: !firrtl.uint<1>) attributes {convention = #firrtl<convention scalarized>} {
    %bar_clock, %bar_a, %bar_b, %bar_c = firrtl.instance bar @Bar(in clock: !firrtl.clock, in a: !firrtl.uint<1>, out b: !firrtl.probe<uint<1>, @A>, out c: !firrtl.uint<1>)
    firrtl.matchingconnect %bar_clock, %clock : !firrtl.clock
    firrtl.matchingconnect %bar_a, %a : !firrtl.uint<1>
    firrtl.matchingconnect %c, %bar_c : !firrtl.uint<1>
  }
}

This has a path through a layerblock via a layer-colored probe. I have an out-of-tree change that I will push that makes layerblock ops RecursiveMemoryEffects and RecursivelySpeculatable (which they arguably should be). With that change, if I do a post-order walk, then I will delete the layerblock op since it is trivially dead after all the operations are removed (which are not side effect free as defined in ODS). If I do a pre-order walk, then I will not delete the layerblock op.

Given the way that we've defined FIRRTL operations, the post-order walk seems to be the only way to do this if we want to delete the layerblock as part of this pass as opposed to relying on something else to delete it, e.g., a canonicalizer.

h/t @rwy7 for working through this example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to add the missing traits to layerblocks here: 8f8acc2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test here showing this example: 08c3c8a

seldridge marked this conversation as resolved.
Show resolved Hide resolved
[&](Operation *op) {
// Connects to values that we found to be dead can be dropped.
LLVM_DEBUG(llvm::dbgs() << "Visit: " << *op << "\n");
if (auto connect = dyn_cast<FConnectLike>(op)) {
if (isAssumedDead(connect.getDest())) {
LLVM_DEBUG(llvm::dbgs() << "DEAD: " << connect << "\n";);
connect.erase();
++numErasedOps;
}
return;
}

// Delete dead wires, regs, nodes and alloc/read ops.
if ((isDeclaration(op) || !hasUnknownSideEffect(op)) &&
isAssumedDead(op)) {
LLVM_DEBUG(llvm::dbgs() << "DEAD: " << *op << "\n";);
assert(op->use_empty() && "users should be already removed");
op->erase();
++numErasedOps;
return;
}

// Remove non-sideeffect op using `isOpTriviallyDead`.
if (mlir::isOpTriviallyDead(op)) {
op->erase();
++numErasedOps;
}
});
}

void IMDeadCodeElimPass::rewriteModuleSignature(FModuleOp module) {
Expand Down
55 changes: 55 additions & 0 deletions test/Dialect/FIRRTL/imdce.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,3 +617,58 @@ firrtl.circuit "OMIRRemoval" {
]} : !firrtl.uint<4>
}
}

// -----

// Test that an operation with a nested block user will be removed (and not
// crash). This should work for both FIRRTL operations and non-FIRRTL
// operations.
//
// CHECK-LAEBL: "Foo"
firrtl.circuit "Foo" {
firrtl.layer @A bind {}
sv.macro.decl @B["B"]
// CHECK-NOT: @Bar
firrtl.module private @Bar() {}
firrtl.module @Foo() {
// CHECK-LABEL: firrtl.layerblock @A
firrtl.layerblock @A {
// CHECK-NOT: firrtl.instance
firrtl.instance bar @Bar()
// CHECK-LABEL: sv.ifdef @B
sv.ifdef @B {
// CHECK-NOT: firrtl.instance
firrtl.instance bar2 @Bar()
}
}
}
}

// -----

// CHECK-LABEL: "Foo"
firrtl.circuit "Foo" {
firrtl.layer @A bind {}
// CHECK-LABEL: @Bar
// CHECK-NOT: out %probe
firrtl.module private @Bar(
in %a: !firrtl.uint<1>,
out %b: !firrtl.uint<1>,
out %probe: !firrtl.probe<uint<1>, @A>
) {
// CHECK-NOT: firrtl.layerblock
firrtl.layerblock @A {
%1 = firrtl.not %a : (!firrtl.uint<1>) -> !firrtl.uint<1>
%a_not = firrtl.node %1 : !firrtl.uint<1>
%2 = firrtl.ref.send %a_not : !firrtl.uint<1>
%3 = firrtl.ref.cast %2 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
firrtl.ref.define %probe, %3 : !firrtl.probe<uint<1>, @A>
}
firrtl.matchingconnect %b, %a : !firrtl.uint<1>
}
firrtl.module @Foo(in %a: !firrtl.uint<1>, out %b: !firrtl.uint<1>) {
%bar_a, %bar_b, %bar_probe = firrtl.instance bar @Bar(in a: !firrtl.uint<1>, out b: !firrtl.uint<1>, out probe: !firrtl.probe<uint<1>, @A>)
firrtl.matchingconnect %bar_a, %a : !firrtl.uint<1>
firrtl.matchingconnect %b, %bar_b : !firrtl.uint<1>
}
}
Loading