[RFC] Making terminator optional (for single block / graph regions)

MLIR
1

This was discussed in a recent ODM, the requirement for every block to have a terminator is an artifact of the evolution of MLIR from a SSA-CFG centric representation to a more generic IR.

This lead to operations like ModuleOp to introduce a companion terminator operation which is never displayed, but has to be explicitly handled. For example the codebase has code like this in many places:
target->addLegalOp<ModuleOp, ModuleTerminatorOp>();
Or even entire patterns:

class ModuleEndConversionPattern
    : public SPIRVToLLVMConversion<spirv::ModuleEndOp> {
public:
  using SPIRVToLLVMConversion<spirv::ModuleEndOp>::SPIRVToLLVMConversion;
  LogicalResult
  matchAndRewrite(spirv::ModuleEndOp moduleEndOp, ArrayRef<Value> operands,
                  ConversionPatternRewriter &rewriter) const override {
    rewriter.replaceOpWithNewOp<ModuleTerminatorOp>(moduleEndOp);
    return success();
  }
};

I have patch that introduce the possibility to make terminator optional: https://reviews.llvm.org/D98468

This supposed that the region has a single block and it is intended to be used for GraphRegion. The patch is fairly large but only because to validate the feature I actually applied it to MLIR ModuleOp and deleted ModuleTerminatorOp from the codebase, i.e. ModuleOp does not define this trait SingleBlockImplicitTerminator<"ModuleTerminatorOp"> anymore and instead append the newly available NoTerminator.traits (see the diff in BuiltinOps.td.

If/when we land this, we can then update all the other flavors of Module (SPIRV and GPU upstream) to remove their terminator, but also other region operations like the FunctionLibraryOp in the shape dialect, which has its own ShapeFunctionLibraryTerminatorOp described as “A pseudo op that marks the end of a shape function library”.

2 Likes
2

Assuming that we can make the codebase reasonably resilient to non-terminator blocks and empty blocks, which when I looked very recently seems to be the case, I’m +1 on doing this. It would be nice to better support this class of operations (using the same line of logic as when graph regions were originally added).

– River

3

This is fantastic, thank you for pushing on this Mehdi!

-Chris

4

One other +1 for this is that it will reduce memory usage for cases that use this. That is perhaps less important for module :wink: but can matter for structure control flow things like sv.if etc.

5

Haven’t heard any concerns so far, so I’ll likely land this on Monday! I’ll avoid “breaking the world” on a Friday afternoon :slight_smile:

6

looking forward to it!

7

Landed! Let me know if you have difficulties to upgrade downstream. It was fairly mechanical and not very involved internally for us.

8

Awesome! I filed an issue for us to adopt it on the circt side of things. Thank you!

-Chris

9

how do I use this new feature? have a few ops that could benefit. (link to patch?)

10

The patch is linked in the first message, here it is again: https://reviews.llvm.org/D98468

For a GraphRegion it is fairly simple, you can look at the ModuleOp: llvm-project/BuiltinOps.td at main · llvm/llvm-project · GitHub

Otherwise you can just add the NoTerminator trait to your operation, it applies to all region but requires the SingleBlock trait.

There is nothing more! Let me know if you need anything :slight_smile:

2 Likes
11

I didn’t make the connection, but this eliminates the need for the ensureTerminator dance for these ops. Beautiful!

2 Likes