This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add support for operation-produced successor arguments in BranchOpInterface
ClosedPublic

Authored by zero9178 on Apr 4 2022, 12:38 PM.

Details

Summary

This patch revamps the BranchOpInterface a bit and allows a proper implementation of what was previously getMutableSuccessorOperands for operations, which internally produce arguments to some of the block arguments. A motivating example for this would be an invoke op with a error handling path:

invoke %function(%0)
  label ^success ^error(%1 : i32)

^error(%e: !error, %arg0 : i32):
  ...

The advantages of this are that any users of BranchOpInterface can still argue over remaining block argument operands (such as %1 in the example above), as well as make use of the modifying capabilities to add more operands, erase an operand etc.

The way this patch implements that functionality is via a new class called SuccessorOperands, which is now returned by getSuccessorOperands. It basically contains an unsigned denoting how many operator produced operands exist, as well as a MutableOperandRange, which are the usual forwarded operands we are used to. The produced operands are assumed to the first few block arguments, followed by the forwarded operands afterwards. The role of SuccessorOperands is to provide various utility functions to modify and query the successor arguments from a BranchOpInterface.


Some sort of breaking changes that are worth discussing:

  • According to the documentation the old return type of Optional<MutableOperandRange> was simply made to temporarily accommodate above cases. Since these would now be properly handled I did not use Optional<SuccessorOperands>. That is technically a loss of functionality as an Op could previously implement BranchOpInterface without having any successor operands at all. I am not sure whether that is an important use case.
  • For now I removed getMutableSuccessorOperands in favour of using getSuccessorOperands. The history as to why both of these exist is very unclear to me and I would be open to separating them again and then probably make a templated version of SuccessorOperands that operates of OperandRange instead, if desired.

Diff Detail

Event Timeline

zero9178 created this revision.Apr 4 2022, 12:38 PM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Apr 4 2022, 12:38 PM
Mogball requested changes to this revision.Apr 4 2022, 2:20 PM
Mogball added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
25

From the description, I wasn't able to easily tell the difference between "produced" arguments and "forwarded" block arguments.

29

Can you include the example you have in the description?

34

Add a constructor SuccessorOperands(MutableOperandRange)?

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
67–68

The example and description you gave in the patch description should be included here.

This revision now requires changes to proceed.Apr 4 2022, 2:20 PM
rriddle requested changes to this revision.Apr 4 2022, 4:54 PM

Seems like a good approach in general and a nice cleanup. It does, however, place restrictions on how operations implementing BranchOpInterface must handle inner-produced vs. forward operands, and that will need to be explicitly conveyed in the interface documentation.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
25

I would make it explicit the order of produced and fowarded operands, and add IR examples here.

34

+1, not having produced operands is overwhelmingly the common case.

43

Add a doc comment here.

45

Prefer active voice for functions.

61

Why the static cast here?

65

Missing punctuation at the end.

84

Please add punctutation.

mlir/test/Transforms/sccp.mlir
204

Please indent this.

zero9178 updated this revision to Diff 420799.Apr 6 2022, 5:12 AM

Address reviewer comments:

  • Add constructor taking just a forwarded operands range
  • Add more documentation to both the SuccessorOperands class and the BranchOpInterface
  • Address comment style issues.
zero9178 marked 11 inline comments as done.Apr 6 2022, 5:13 AM
zero9178 added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
61

MutableOperandRange does not have a operator[]. Doing the static_cast calls the conversion operator to OperandRange however, which does.

rriddle accepted this revision.Apr 6 2022, 5:37 PM

Nice work, thanks!

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
61

Can you just add an operator[] to MutableOperandRange?

zero9178 updated this revision to Diff 421153.Apr 7 2022, 3:59 AM
zero9178 marked an inline comment as done.

Define and make use of operator[] in MutableOperandRange

zero9178 marked an inline comment as done.Apr 7 2022, 3:59 AM
Mogball accepted this revision.Apr 7 2022, 10:22 AM
This revision is now accepted and ready to land.Apr 7 2022, 10:22 AM