Page MenuHomePhabricator

[MLIR] Allow Loop dialect IfOp and ForOp to define values
AcceptedPublic

Authored by nmostafa on Thu, Feb 6, 2:59 PM.

Details

Summary

This patch implements the RFCs proposed here and here.
It introduces the following changes:

  • All Loop Ops region, except for ReduceOp, terminate with a YieldOp.
  • YieldOp can have variadice operands that is used to return values out of IfOp and ForOp regions.
  • Change IfOp and ForOp syntax and representation to define values.
  • Add unit-tests and update .td documentation.
  • YieldOp is a terminator to loop.for/if/parallel
  • YieldOp custom parser and printer

Lowering is not supported at the moment, and will be in a follow-up PR.

Thanks.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rriddle added inline comments.Thu, Feb 6, 3:37 PM
mlir/lib/Dialect/LoopOps/LoopOps.cpp
89

You need to return when you emit an error.

158

Don't use !, use succeeded/failed(...) instead when parsing optional constructs.

501

Why is this necessary?

510

terminates

518

Return on error.

528

Start sentences with capital letters. Also typo opreands.

This revision now requires changes to proceed.Thu, Feb 6, 3:37 PM
nmostafa updated this revision to Diff 243062.Thu, Feb 6, 5:27 PM
nmostafa marked 14 inline comments as done.

Review fixes

nmostafa marked 2 inline comments as done.Thu, Feb 6, 5:34 PM
nmostafa added inline comments.
mlir/lib/Dialect/LoopOps/LoopOps.cpp
158–168

I personally prefer seeing the types right next to the operands for better readability (especially if you have too many operands, you have to look at the end of the op to figure out the types).

The result types are still convenient look at to quickly figure out the return types, but I am happy to remove it if there is consensus.

501

EnsureTerminator expects such a builder interface. Even though YieldOp has variadic arguments, ODS doesn't auto-gen that builder. Looks like ODS limitation.

bondhugula requested changes to this revision.Thu, Feb 6, 7:13 PM

Mostly typos and comment fixes.

mlir/include/mlir/Dialect/LoopOps/LoopOps.td
54–56

such a region

67

Nit: will have -> has

72

Nit: result variables -> results

198

Better if this is rephrased to:

"Also, if "loop.if" defines one or more values, the 'else' block cannot be omitted."

258–259

such a region

360

Consider rephrasing to:
"If ..., the operands must match the parent op's results."

mlir/include/mlir/IR/OpImplementation.h
649

Terminate comments with a period.

649

(%x1 = %y1 : type1, %x2 = %y2 : type2, ...)

to be clearer here.

mlir/lib/Dialect/LoopOps/LoopOps.cpp
84

Nit: defines
comma after values. "that number" -> "that the number"
of of defined -> of the defined

153

Terminate comments with periods please.

This revision now requires changes to proceed.Thu, Feb 6, 7:13 PM
nicolasvasilache accepted this revision.Fri, Feb 7, 9:01 AM

Thanks @nmostafa for pushing on this important feature, it will make everyone's life better.
Looking forward to seeing this lower to LLVM all the way to execution!

nmostafa updated this revision to Diff 243194.Fri, Feb 7, 9:26 AM
nmostafa marked 10 inline comments as done.

Fix comments and documentation per review feedback.

rriddle added inline comments.Fri, Feb 7, 10:31 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
368–371

Why are you not specifying a printer? Also, can you just use the declarative form? That would remove the need for defining the parser/printer in c++.

https://mlir.llvm.org/docs/OpDefinitions#declarative-assembly-format

Should be something like:

let assemblyFormat = "operands attr-dict `:` type(operands)";
369

Can you just provide the body of the builder here instead of the .cpp?

mlir/lib/Dialect/LoopOps/LoopOps.cpp
88

Can you switch this to early exit instead?

159

This needs to be checked for failure, missing tests?

nmostafa updated this revision to Diff 243309.Fri, Feb 7, 3:54 PM
nmostafa marked 2 inline comments as done.
nmostafa marked 2 inline comments as done and an inline comment as not done.Fri, Feb 7, 3:56 PM
nmostafa added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
368–371

The original TerminatorOp didn't have a custom printer, so I just stuck with that.

If you feel strongly about it, I can change it. However, using assemblyFormat will yield an awkward printing when there are no operands: loop.yield : and parsing will fail if the colon is missing. Is there a way to specify optional tokens in the declarative form ?

mlir/lib/Dialect/LoopOps/LoopOps.cpp
159

What needs to be checked ? iter_args in the syntax ?

rriddle added inline comments.Fri, Feb 7, 4:18 PM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
368–371

Oops forgot about the variadic operands bit, sorry about that. You will need to use c++ for that for now.

The original TerminatorOp didn't have a custom printer, so I just stuck with that.

That's because it was never printed. Now it can be, so it actually needs a printer. If you are defining a parser, you need a printer.

bondhugula accepted this revision.Fri, Feb 7, 4:25 PM
bondhugula marked an inline comment as done.
bondhugula added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
368–371

The original TerminatorOp didn't have a custom printer, so I just stuc

Sounds good to me. The custom printer can be done in a separate patch. You can add a TODO - it would be good to have a custom print for this now that it's elided less (having quotes around it would look weird!).

mlir/test/Dialect/Loops/ops.mlir
112

extra space added here

rriddle added inline comments.Fri, Feb 7, 4:33 PM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
368–371

The custom printer should be added in the same patch as the parser, whether it be this one or the next. I'm not comfortable with this going in with only one defined.

bondhugula added inline comments.Sat, Feb 8, 4:52 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
368–371

Okay. In that case, the commit message should be updated to reflect that (addition of custom parser/printer for yield).

Thanks! This looks great. I had written some comments but forgot to hit the submit button it seems :(

mlir/include/mlir/Dialect/LoopOps/LoopOps.td
126

Should we add a builder with support for iter_args as well?

152

Should this just be a static constexpr unsigned kNumCtrlOperands?

258–259

Mention that the yield may not have operands.

354–355

I wonder whether we should keep the TerminatorOp for the loop.parallel. It is always implicit there anyway and has different semantics (the return values of loop.parallel are defined by loop.reduce operation. Having the extra op is not costing much and avoid the extra checks in verifiers.

mlir/lib/Dialect/LoopOps/LoopOps.cpp
513

Could you also verify that in the case of a Parallel operation the yield does not have any operands?

nmostafa updated this revision to Diff 243660.Mon, Feb 10, 1:04 PM
nmostafa marked an inline comment as done.
nmostafa edited the summary of this revision. (Show Details)
  • Added custom printer for YieldOp
  • Added tests to verify loop.parallel yield has no operands. Negative test for that as well.
  • Updated td description for loop.parallel: yield without opreands
  • Updates to commit message
nmostafa marked 3 inline comments as done.Mon, Feb 10, 1:09 PM
nmostafa added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
126

To build an empty loop with no updates to the init args ? Otherwise, the loop will be invalid if the YieldOp has no operands.
Seems a bit awkward to me as opposed to generating an empty loop with no variables and have the user add variables to the ForOp, Region and YieldOp.

368–371

Actually, the TerminatorOp was printed before with the default printer.
ops.mlir: // CHECK: "loop.terminator"() : () -> ()

The only difference is that yield now can be explicit in the source and that's why a custom parser makes sense.
Let me add a custom printer and change the tests.

nmostafa marked an inline comment as done.Mon, Feb 10, 1:13 PM
nmostafa added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
354–355

There are mixed opinions on this .. see https://llvm.discourse.group/t/rfc-adding-operands-and-results-to-loop-for/459/14
IMO, unifying under one terminator makes more sense, and yield semantics is really: Terminate and Yield values.

This looks like a really great start! Thanks for sketching this out.

mlir/lib/Dialect/LoopOps/LoopOps.cpp
158–168

@ftynse also suggested leaving off the return type for the for loop. Anyone else have an opinion on this? I'd really prefer for() and if() to be consistent in the respect, so I think in this incarnation, either the syntax for for() or if() needs to change.

I think the whole representation and syntax of loop.reduce / modeling reductions inside loop.for could be improved. Was this already discussed somewhere? If not, can we initiate one?

mlir/include/mlir/Dialect/LoopOps/LoopOps.td
263

Is this missing a return value assignment for its result for clarity?

I think we could add a reduction_vars / reduction_args list just like iter_args on loop.for and completely get rid of loop.reduce / loop.reduce.return and just use yield from within for's region to send out both reduction vars and general cross loop/loop live out vars.

How difficult is it to support both the old-style "does not return a value" and the new terminator that does return a value?

mlir/lib/Dialect/LoopOps/LoopOps.cpp
288

The comment here doesn't seem necessary.

296

Likewise here.

nmostafa marked an inline comment as done.Tue, Feb 11, 11:54 AM

How difficult is it to support both the old-style "does not return a value" and the new terminator that does return a value?

Not difficult, but since the new YieldOp terminator optionally return a value, it can serve both purposes.

mlir/lib/Dialect/LoopOps/LoopOps.cpp
158–168

We still need the return type for the IfOp, not sure how omitting it for ForOp would help consistency. Maybe I am not sure I follow the IfOp format you are suggesting:

if %c args(%x=%y: i32) {...} else {...}

Unlike ForOp, there is no binding needed upon entering the IfOp regions. Can you re-write the example below using the format you are suggesting ?

%d = if %c -> (f32) {  
   %r1 = ... : f32
    yield %r1 : f32
} else {  
   %r2 = ... : f32
    yield %r2 : f32
}

I am mostly nitpicking. Let's finish the design discussion on discourse and land this!

mlir/include/mlir/Dialect/LoopOps/LoopOps.td
81

Random observation: we mention the same type twice in iter_args and in the trailing arrow-type, which are also located close. Did you consider omitting the trailing type?

152

Nit: can we also spell out Control? It only needs 3 more characters.

355

Let's replace cf with loop here. cf dates back to a temporary state when the dialect used the prefix cf for "control flow".

360

"the operands must match..." should yield enforce this or should the parent operations enforce this?

362

Nit: it is _always_ present, we just omit it while pretty printing. Could you please rephrase this text to reflect that fact? Otherwise, it creates an incentive to check for the presence when working with loop.if/for in the code.

mlir/lib/Dialect/LoopOps/LoopOps.cpp
92

Nit: "op lop-carried" sounds weird

102

Mega-nit: I find it helpful to print the index of the mismatching type

130

Doesn't the presence of iter operands imply there should be results?

158–168

I don't have a strong opinion and I am receptive to the argument of consistency, i.e. for and if should have as close syntax as possible.

nmostafa marked an inline comment as done.Tue, Feb 11, 3:30 PM
nmostafa added inline comments.
mlir/lib/Dialect/LoopOps/LoopOps.cpp
158–168

I will omit the types inside iter_args in ForOp. To become:
for()....iter_args(%x=%y) -> (i32) {...}

nmostafa updated this revision to Diff 244044.Tue, Feb 11, 5:41 PM
  • Remove types from iter_args assignment list.
  • Fix other nits per review.
rriddle accepted this revision.Mon, Feb 17, 7:40 PM
rriddle added inline comments.
mlir/lib/Dialect/LoopOps/LoopOps.cpp
123

nit:

llvm::interleaveComma(llvm::zip(...), p, [&](auto it) {
  p << ...
}); 
p << ")";
127

Why assert here? This seems like something that would already be verified.

519

nit: Add {} for each if/else clause if any of them have one.

This revision is now accepted and ready to land.Mon, Feb 17, 7:40 PM
nmostafa updated this revision to Diff 245222.Tue, Feb 18, 11:36 AM
nmostafa marked an inline comment as done.

Rebased.
Use mlir::interleaveComma and remove assertion.

nmostafa updated this revision to Diff 245225.Tue, Feb 18, 11:40 AM

Add missing {} around else