-
Notifications
You must be signed in to change notification settings - Fork 16.1k
[mlir][Interfaces] Split successor inputs from region successor #175815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][Interfaces] Split successor inputs from region successor #175815
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. I've approved it.😉
linuxlonelyeagle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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 <[email protected]>
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