Skip to content
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

Windows python interop.h #1397

Closed
wants to merge 10 commits into from
Closed

Conversation

oroppas
Copy link

@oroppas oroppas commented Sep 22, 2022

Fixes #1301

@ashay
Copy link
Collaborator

ashay commented Sep 22, 2022

Thanks for these changes @oroppas! Just so that we don't duplicate work between the two of us, I wanted to let you know about PR #1376 that applies a few other fixes necessary to get the build working on Windows, in case you find those useful!

@oroppas
Copy link
Author

oroppas commented Sep 23, 2022

@ashay I'm so sorry. I was completely unaware of #1376.
I'll close this PR. Just to make sure, #1376 does not incorporate the change of #1310?

@ashay
Copy link
Collaborator

ashay commented Sep 23, 2022

Oh hey @oroppas! Don't close this PR, since it still has useful changes! I just wanted to make sure that we could share the work involved.

No need to apologize, and thanks for your contributions. They are much appreciated!

@oroppas
Copy link
Author

oroppas commented Sep 23, 2022

Hi @ashay. OK. I keep this PR open. I also make sure to pay attention to #1376. I will open an issue/PR and keep you in the loop so that I find can complement your effort. Thank you!

@ashay
Copy link
Collaborator

ashay commented Sep 23, 2022

My apologies, I forgot to answer your question about #1310 versus #1376. You're right that #1376 does not include the changes in #1310, so we should definitely leave #1310 open.

However, are changes in this PR covered by #1376 and #1310? If so, we should close this PR. Sorry for not grasping that earlier.

@oroppas
Copy link
Author

oroppas commented Sep 23, 2022

Hi @ashay. Thank you for the clarification. Closing this PR as covered by #1376.

@oroppas oroppas closed this Sep 23, 2022
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
According to https://llvm.org/docs/LangRef.html#alloca-instruction, the `alloca` instruction allocates memory on the stack frame of the currently executing function, to be automatically released when this function returns to its caller. 

We can avoid over allocing by placing `alloca` instruction at the function scope and outside of any loops. This change is motivated by onnx/onnx-mlir#1061, where models segfault at runtime when compiling at `-O0`, due to run out of stack space.

Signed-off-by: Whitney Tsang <whitneyt@ca.ibm.com>
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
According to https://llvm.org/docs/LangRef.html#alloca-instruction, the alloca instruction allocates memory on the stack frame of the currently executing function, to be automatically released when this function returns to its caller.

We can avoid over allocing by placing alloca instruction at the function scope and outside of any loops.

llvm#1397 and llvm#1437 attempted to create the allocas outside of loops directly. However, it is not always possible to create outside of the whole loop nest. For example, when lowering an operation that is already in a loop nest, or when the outer loop is only created after the placement of the alloca.

This PR adds BufferLoopHoistingPass after LowerAffinePass to hoist allocations out of loop nests to avoid stack overflow.

Signed-off-by: Whitney Tsang whitneyt@ca.ibm.com
powderluv added a commit to powderluv/iree that referenced this pull request Oct 10, 2022
It breaks windows builds when it is included.

Similar fix went into torch-mlir llvm/torch-mlir#1397
powderluv added a commit to powderluv/iree that referenced this pull request Oct 10, 2022
It breaks windows builds when it is included.

Similar fix went into torch-mlir llvm/torch-mlir#1397
hanhanW pushed a commit to iree-org/iree that referenced this pull request Oct 14, 2022
It breaks windows builds when it is included.

Similar fix went into torch-mlir
llvm/torch-mlir#1397
PhaneeshB pushed a commit to PhaneeshB/iree that referenced this pull request Oct 31, 2022
It breaks windows builds when it is included.

Similar fix went into torch-mlir
llvm/torch-mlir#1397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python bindings build failure on Windows: python/TorchMLIRModule.cpp
2 participants