[mlir][Interfaces] Split successor inputs from region successor#175815
[mlir][Interfaces] Split successor inputs from region successor#175815matthias-springer merged 1 commit intomainfrom
Conversation
4976ae4 to
792ea47
Compare
792ea47 to
6feec83
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
6b144e4 to
072b98c
Compare
f09ea5a to
cc13c77
Compare
zero9178
left a comment
There was a problem hiding this comment.
LGTM, but please give it a few days for others to review as well
cc13c77 to
bf8c119
Compare
| branch->getResults().slice(firstIndex, inputs.size())), | ||
| lattices, firstIndex); | ||
| branch, RegionSuccessor::parent(), | ||
| branch->getResults().slice(firstIndex, inputs.size()), lattices, |
There was a problem hiding this comment.
Could we avoid adding an extra argument to visitNonControlFlowArgumentsImpl?"Since this PR implements getSuccessorInputs, I think we could potentially leverage it in the subsequent call.
There was a problem hiding this comment.
Maybe. I am trying to do that here: #175978. But I don't know that part of the code base well enough. I'm not sure if that's safe.
This PR is already large enough, so I wanted to limit the changes to the "mechanical" ones, for which we can for sure say that they are correct. Then improve that API (and maybe others) in a follow-up commit.
There was a problem hiding this comment.
I see, thanks. I've approved it.😉
bf8c119 to
3b1d7ed
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/37130 Here is the relevant piece of the build log for the reference |
Fix this build error, which is reported by some compilers after #175815: ``` error: operands to ?: have different types ‘mlir::Operation::result_range {aka mlir::ResultRange}’ and ‘mlir::ValueRange’ return successor.isParent() ? getOperation()->getResults() : ValueRange(); ```
…#175815) This commit simplifies the design of the `RegionBranchOpInterface`. The property of being a successor input is now independent of the region branch point. There is a new API for querying successor inputs: `RegionBranchOpInterface::getSuccessorInputs(RegionSuccessor)`. Note that this function does **not** take a `RegionBranchPoint` as parameter. The `RegionSuccessor` API is now also simpler: it no longer stores successor inputs. A region successor is simply `Region *`, wrapped around a convenience API. Note: This commit is mostly mechanical. Analyses / transformations that build on top of the `RegionBranchOpInterface` (e.g., `visitNonControlFlowArguments` API) can likely be simplified in follow-up commits. Note for LLVM integration: Split `RegionBranchOpInterface::getSuccessorRegion` implementations into two functions: `getSuccessorRegion` and `getSuccessorInputs. (There are many examples in this commit.) RFC: https://discourse.llvm.org/t/rfc-simplify-regionbranchopinterface-separate-successor-inputs-from-region-successor/89420/7
Fix this build error, which is reported by some compilers after llvm#175815: ``` error: operands to ?: have different types ‘mlir::Operation::result_range {aka mlir::ResultRange}’ and ‘mlir::ValueRange’ return successor.isParent() ? getOperation()->getResults() : ValueRange(); ```
…#175815) This commit simplifies the design of the `RegionBranchOpInterface`. The property of being a successor input is now independent of the region branch point. There is a new API for querying successor inputs: `RegionBranchOpInterface::getSuccessorInputs(RegionSuccessor)`. Note that this function does **not** take a `RegionBranchPoint` as parameter. The `RegionSuccessor` API is now also simpler: it no longer stores successor inputs. A region successor is simply `Region *`, wrapped around a convenience API. Note: This commit is mostly mechanical. Analyses / transformations that build on top of the `RegionBranchOpInterface` (e.g., `visitNonControlFlowArguments` API) can likely be simplified in follow-up commits. Note for LLVM integration: Split `RegionBranchOpInterface::getSuccessorRegion` implementations into two functions: `getSuccessorRegion` and `getSuccessorInputs. (There are many examples in this commit.) RFC: https://discourse.llvm.org/t/rfc-simplify-regionbranchopinterface-separate-successor-inputs-from-region-successor/89420/7
Fix this build error, which is reported by some compilers after llvm#175815: ``` error: operands to ?: have different types ‘mlir::Operation::result_range {aka mlir::ResultRange}’ and ‘mlir::ValueRange’ return successor.isParent() ? getOperation()->getResults() : ValueRange(); ```
Integrate llvm at 3ca2a5fc0b84762f0e7d8a0e613fd69f7e344219 This includes the **RegionBranchOpInterface** related changes from llvm/llvm-project#175815
This reverts commit 8ca6c8f13398c5bbe961e9bc874d6b3de398e5e8. Also uses `visitNonControlFlowArguments` new API since llvm/llvm-project#175815
…#175815) This commit simplifies the design of the `RegionBranchOpInterface`. The property of being a successor input is now independent of the region branch point. There is a new API for querying successor inputs: `RegionBranchOpInterface::getSuccessorInputs(RegionSuccessor)`. Note that this function does **not** take a `RegionBranchPoint` as parameter. The `RegionSuccessor` API is now also simpler: it no longer stores successor inputs. A region successor is simply `Region *`, wrapped around a convenience API. Note: This commit is mostly mechanical. Analyses / transformations that build on top of the `RegionBranchOpInterface` (e.g., `visitNonControlFlowArguments` API) can likely be simplified in follow-up commits. Note for LLVM integration: Split `RegionBranchOpInterface::getSuccessorRegion` implementations into two functions: `getSuccessorRegion` and `getSuccessorInputs. (There are many examples in this commit.) RFC: https://discourse.llvm.org/t/rfc-simplify-regionbranchopinterface-separate-successor-inputs-from-region-successor/89420/7
Fix this build error, which is reported by some compilers after llvm#175815: ``` error: operands to ?: have different types ‘mlir::Operation::result_range {aka mlir::ResultRange}’ and ‘mlir::ValueRange’ return successor.isParent() ? getOperation()->getResults() : ValueRange(); ```
This reverts commit 8ca6c8f13398c5bbe961e9bc874d6b3de398e5e8. Also uses `visitNonControlFlowArguments` new API since llvm/llvm-project#175815 Signed-off-by: Keshav Vinayak Jha <keshavvinayakjha@gmail.com>
This commit simplifies the design of the
RegionBranchOpInterface. The property of being a successor input is now independent of the region branch point.There is a new API for querying successor inputs:
RegionBranchOpInterface::getSuccessorInputs(RegionSuccessor). Note that this function does not take aRegionBranchPointas parameter.The
RegionSuccessorAPI is now also simpler: it no longer stores successor inputs. A region successor is simplyRegion *, wrapped around a convenience API.Note: This commit is mostly mechanical. Analyses / transformations that build on top of the
RegionBranchOpInterface(e.g.,visitNonControlFlowArgumentsAPI) can likely be simplified in follow-up commits.Note for LLVM integration: Split
RegionBranchOpInterface::getSuccessorRegionimplementations into two functions:getSuccessorRegionand `getSuccessorInputs. (There are many examples in this commit.)RFC: https://discourse.llvm.org/t/rfc-simplify-regionbranchopinterface-separate-successor-inputs-from-region-successor/89420/7