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

[AMDAIECoreToStandard] Improve error message when core not isolated #718

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

newling
Copy link
Contributor

@newling newling commented Aug 28, 2024

In case we hit this error again, I'd like the message to be more informative. The new error is something like:

<unknown>:0: error: 'arith.constant' op is not in the core in which it is used. Cores must be `isolated` before this point.
<unknown>:0: note: see current operation: %0 = "arith.constant"() <{value = 0 : i32}> : () -> i32
<unknown>:0: error: 'aie.device' op Failed to lower to LLVM
<unknown>:0: note: see current operation:
"aie.device"() <{device = 8 : i32}> ({

compared to old error, something like:

LLVM ERROR: operation destroyed but still has uses
Please report issues to https://github.com/iree-org/iree/issues and include the crash backtrace.
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  libIREECompiler.so 0x00007fa09d494f87 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 39
1  libIREECompiler.so 0x00007fa09d4931c0 llvm::sys::RunSignalHandlers() + 80

@newling newling requested a review from makslevental as a code owner August 28, 2024 21:03
Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -251,6 +257,28 @@ struct AMDAIECoreToStandardPass : mlir::OperationPass<ModuleOp> {
if (failed(applyPartialConversion(m, target, std::move(patterns))))
return signalPassFailure();

// Assert that cores are isolated
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could put it in a utility function inside the CoreOp itself so it can be reused in different places. But I wouldn't spend effort on that for AIE ops and wait for it until we solely rely on AMDAIE ops.

@newling newling merged commit b7418ee into nod-ai:main Aug 29, 2024
5 checks passed
@newling newling deleted the improved_core_isolation_error branch August 29, 2024 18:26
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.

2 participants