lower if #1693
Merged
lower if #1693
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sorenlassen
force-pushed
the
sorenlassen/lower-if
branch
from
dd37ae9 to
2c0796d
Compare
|
@chentong319 can you review this PR as it is closely related to the LOOP and sequence work you have done, big thanks |
added lit tests and backend test updated SupportedONNXOps-cpu.md Signed-off-by: Soren Lassen <[email protected]>
sorenlassen
force-pushed
the
sorenlassen/lower-if
branch
from
2c0796d to
4a93bb8
Compare
chentong319
reviewed
Sep 19, 2022
| rewriter, loc, ifOp.then_branch(), scfIfOp.getThenRegion()); | ||
| graphToScfBranch( | ||
| rewriter, loc, ifOp.else_branch(), scfIfOp.getElseRegion()); | ||
| UnrealizedConversionCastOp castOp = |
There was a problem hiding this comment.
To my understanding, this statement converts the result of scf.if back to tensor types. But the lit test case shows the return is memref. Can you tell me the reason?
There was a problem hiding this comment.
good point, the cast was unnecessary
removed
Signed-off-by: Soren Lassen <[email protected]>
|
I noticed that after this change the lit tests no longer pass on Windows in Debug. Windows Debug is often special in that it explicitly sets everything to null if it's not initialized, pads structures with known patterns (and other similar things) to more explicitly guard against bugs. In this case, there is an access violation in the new test for lowering |
Merged
I only have a Mac and can only test on Mac and Linux so I can't reproduce the failure thanks for sharing the backtrace, I can't conclusively say what the issue is but my hunch is that it has to do with the way the returnOp's block was erased so I wrote PR #1717 as an experiment - are you able to run that change to see if it fixes the issue? or can you recommend how I can troubleshoot this? |
@sstamenova can you clarify for my education why this was not picked up by the normal CIs? Window does not test in Debug? Tx |
Right now we don't run a debug build in the Windows CI. They are longer and use more memory, so it may or may not work on the available machines. We can try it out and see what happens. |
|
@sorenlassen : The new changes fail on Windows and Ubuntu both. The callstack for Ubuntu: The callstack for Windows: |
sstamenova
pushed a commit
to sstamenova/onnx-mlir
that referenced
this pull request
Sep 21, 2022
* lower if added lit tests and backend test updated SupportedONNXOps-cpu.md Signed-off-by: Soren Lassen <[email protected]> * remove unnecessary cast Signed-off-by: Soren Lassen <[email protected]> Signed-off-by: Soren Lassen <[email protected]> Co-authored-by: chentong319 <[email protected]> Signed-off-by: Stella Stamenova <[email protected]>
|
@sstamenova I updated my attempted fix in PR #1717, can you please try to run the Windows Debug lit tests with that PR again? |
All green now. Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
added lit tests and backend test
updated SupportedONNXOps-cpu.md
Signed-off-by: Soren Lassen [email protected]