Page MenuHomePhabricator

[mlir] Add support for merging identical blocks during canonicalization
ClosedPublic

Authored by rriddle on Apr 29 2020, 2:57 PM.

Details

Summary

This revision adds support for merging identical blocks, or those with the same operations that branch to the same successors. Operands that mismatch between the different blocks are replaced with new block arguments added to the merged block.

Depends On D79077

Diff Detail

Event Timeline

rriddle created this revision.Apr 29 2020, 2:57 PM
mehdi_amini accepted this revision.May 4 2020, 12:22 PM

The whole handling of mismatchedOperands caused me some mental effort to follow, I think it'd be nice to have a high-level description of this parts (which is more than "identical block" strictly speaking).

mlir/include/mlir/IR/Operation.h
560

I'm surprised this isn't an "any_of" here, it can be surprising with the name of the method, and the difference with the same method on the Value class.

mlir/include/mlir/IR/OperationSupport.h
861

What about LLVM_MARK_AS_BITMASK_ENUM and use the Enum instead of unsigned?

mlir/lib/Transforms/Utils/RegionUtils.cpp
379

can you add a description of what is considered "equivalent"?

392

Can you expand here?
In particular the order here is counting the results of an operation: an operation order in a block is the number of defined value in the block before this operation (including the block arguments)

481

auto->Value?

485

The comment seems misleading to me (or misplaced, I would write this before the line if (!lhsIsInBlock) {

508

Could check this ahead using the block ordering of the terminator?

519

Nit: in general predicate a more readable when they are written with positive logic instead of negative ("able" instead of "unable")

539

You could get this by checking this property in addToCluster and return a failure when there are operandsToMerge so that you don't cluster blocks in the first place

578

I didn't quite get what this meant until I read the code, what about something around the line of: // Update each block's predecessors terminator with the new arguments

579

Do you have some name more explicit than it?

580

Can you use something else than i for the iterator? I'm already not thrilled by i for integer index, but here it seems misleading as it is an iterator that it derefenced.

623

Å->A

This revision is now accepted and ready to land.May 4 2020, 12:22 PM
rriddle updated this revision to Diff 261993.May 4 2020, 7:54 PM
rriddle marked 17 inline comments as done.

Resolve comments

This revision was automatically updated to reflect the committed changes.