This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by nmostafa on Feb 6 2020, 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

nmostafa created this revision.Feb 6 2020, 2:59 PM

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

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

For the for() loop syntax, I think that iter_args and the result type list should go together. They are either both present or both absent. As a result, there should be no need to replicate the type information 2x in the op syntax.

I think this could work two ways:

for()....iter_args(%x=%y) -> (i32) {...}
if %c -> (i32) {...} else {...}

However, with the else branch of the if being optional, it might make sense to have:

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

This would also more conveniently represent the case where %x is assigned in one branch of the if, but not the other.

mlir/test/Dialect/Loops/invalid.mlir
356

Need negative tests for type conflicts.

rriddle requested changes to this revision.Feb 6 2020, 3:37 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
118

Is this wrapped at 80 characters?

142

Can you just do:

getOperands().drop_front(getNumCtrlOperands())

?

147

Capitalize sentences and change these to ///

154

This doesn't look like it's wrapped at 80 characters/

197

A Also?

198

Same here.

208

nit: arguments before results

359

then -> the

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.

510

Why is this necessary?

519

terminates

527

Return on error.

537

Start sentences with capital letters. Also typo opreands.

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

Review fixes

nmostafa marked 2 inline comments as done.Feb 6 2020, 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.

510

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.Feb 6 2020, 7:13 PM

Mostly typos and comment fixes.

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

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."

259

such a region

359

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

mlir/include/mlir/IR/OpImplementation.h
638

Terminate comments with a period.

638

(%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.Feb 6 2020, 7:13 PM
nicolasvasilache accepted this revision.Feb 7 2020, 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.Feb 7 2020, 9:26 AM
nmostafa marked 10 inline comments as done.

Fix comments and documentation per review feedback.

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

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

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)";
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.Feb 7 2020, 3:54 PM
nmostafa marked 2 inline comments as done.
nmostafa marked 2 inline comments as done and an inline comment as not done.Feb 7 2020, 3:56 PM
nmostafa added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
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.Feb 7 2020, 4:18 PM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
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.Feb 7 2020, 4:25 PM
bondhugula marked an inline comment as done.
bondhugula added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
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.Feb 7 2020, 4:33 PM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
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.Feb 8 2020, 4:52 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
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?

259

Mention that the yield may not have operands.

353

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
522

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.Feb 10 2020, 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.Feb 10 2020, 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.

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.Feb 10 2020, 1:13 PM
nmostafa added inline comments.
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
353

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
280

The comment here doesn't seem necessary.

288

Likewise here.

nmostafa marked an inline comment as done.Feb 11 2020, 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.

354

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

359

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

361

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

126

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.Feb 11 2020, 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.Feb 11 2020, 5:41 PM
  • Remove types from iter_args assignment list.
  • Fix other nits per review.
rriddle accepted this revision.Feb 17 2020, 7:40 PM
rriddle added inline comments.
mlir/lib/Dialect/LoopOps/LoopOps.cpp
119

nit:

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

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

512

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

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

Rebased.
Use mlir::interleaveComma and remove assertion.

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

Add missing {} around else

If there are no further comments, going to commit this today.

nmostafa updated this revision to Diff 245900.Feb 21 2020, 9:55 AM

Fix LIT tests after merge from master.

This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Feb 21 2020, 11:10 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
152

This doesn't build with GCC 5, remove the constexpr.

rriddle added inline comments.Feb 21 2020, 11:13 AM
mlir/include/mlir/Dialect/LoopOps/LoopOps.td
152