RFC: Add `ImplicitLocOpBuilder` to Builders.h?

MLIR
1

Hi all,

As @ftynse and others have previously raised, it is a very common pattern to create a bunch of operations with the same location. As such, it can be convenient to have an OpBuilder which is given a location on its construction, and that automatically provides that location to each create<> call within it. This nicely composes with our existing OpBuilder class, and I introduced this (plus an example usage) into the circt repository in this patch.

I’m curious to know what folks think about this - should we introduce such a thing into the mainline mlir/IR/Builders.h file?

One of the specific design points of this is that it keeps the existing OpBuilder type without a location, and it keeps the mainline MLIR infra passing around OpBuilder. This means that implicit location propagation is an “opt in” thing or various passes, not “the default” which encourages location safety.

In any case, I’m curious to know what y’all think, and would also appreciate any suggestions in the API etc.

-Chris

2

Relatedly, this CIRCT patch adds some helper functions to the sv.if/sv.ifdef/sv.alwaysatposedge ops that enable EDSC-style construction of nested operations. I think it would be nice to adopt a similar pattern for affine.if/affine.for ops to make it easier to build them. I think it would also be nice for ODS to autogenerate the builders like this for single-region operations. @River707 have you ever considered such a thing?

Here’s an example from the patch:

  // Emit this into an "sv.alwaysat_posedge" body.
  builder->create<sv::AlwaysAtPosEdgeOp>(clock, [&]() {
    // Emit an "#ifndef SYNTHESIS" guard into the always block.
    builder->create<sv::IfDefOp>("!SYNTHESIS", [&]() {
      // Emit an "sv.if '`PRINTF_COND_ & cond' into the #ifndef.
      Value ifCond = stuff_elided();
      builder->create<sv::IfOp>(ifCond, [&]() {
        // Emit the sv.fwrite.
        builder->create<sv::FWriteOp>(op.formatString(), operands);
      });
    });
  });

which builds this MLIR:

  sv.alwaysatposedge %clock {
    sv.ifdef "!SYNTHESIS" {
      %ifcond = stuff_elided()
      sv.if %ifcond {
        sv.fwrite "formatstring", operands
      }
    }
  }

This seems like a nice and simple pattern that could be useful in many places.

-Chris

3

This sounds good to me. As pointed out by many in the thread on enhanced builders, using a single location while expanding/lowering out a higher level op is a very common use case.

4

I honestly thought we were already generating this for regions. We’ve been doing this for a while (at least in SCF): e.g. llvm-project/mlir/lib/Dialect/SCF/SCF.cpp at 19313ed580af4d851c12c4b7c244795df9967e2f · llvm/llvm-project · GitHub

5

This is a welcome simplification, thanks Chris!

We don’t, SCF and some Linalg ops have custom builders for this case. The main reason why we don’t have that yet is that, at the moment when these builders are added, ODS wasn’t even aware of regions. I would still suggest a minor extension to the ODS modeling of regions to support such builders – somehow list the arguments of the entry block of the region. For builders, we only need their structure, e.g. ForOp body callback separates induction variable from the iteration arguments in (..., Value, ValueRange). If we actually need to create a region, we’d need exact types.

6

I proposed a patch for this here, it has worked well in the CIRCT world.

-Chris