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

Printer: print MISSING_SSA_VALUE instead of abort #119

Merged
merged 8 commits into from
Jun 23, 2022

Conversation

webmiche
Copy link
Collaborator

@webmiche webmiche commented Jun 15, 2022

closes #86. This changes the behavior on missing SSAValues from aborting with an exception to printing %MISSING_SSA_VALUE. While I feel that it makes sense to not abort if not needed, I feel that this current version does not really give insight into what went wrong. Still, how do you guys feel about this change?

Edit: this means that an add with missing operands is printed as:

%0 : !i32 = arith.addi(%MISSING_SSA_VALUE : !i32, %MISSING_SSA_VALUE : !i32)

Instead of failing with a KeyError and the message

SSAValue is not part of the IR, are you sure all operations are added before their uses?

@webmiche
Copy link
Collaborator Author

webmiche commented Jun 15, 2022

The latest commit contains a way to show more information, similar to the earlier error message. It would be really cool if we could combine this more with the diagnostics (to e.g. have the ^^^^ at the right positon). However, the error happens during printing of the operands, while "normal" error messages are added to the next line before printing. Therefore, I do not have access to the positions that this would need.

On way to fix this, might be to allow adding diagnostic messages to SSAValues or doing two separate passes over the IR, the first one adding all these error messages, the second one actually printing. Anyway, I feel that the current version gives us not quite the best of both worlds, but already quite a lot.

Edit: This current version prints as:

%0 : !i32 = arith.addi(%MISSING_SSA_VALUE : !i32, %MISSING_SSA_VALUE : !i32)
^-------------------------------------------------------------------------------------------------
| ERROR: SSAValue is not part of the IR, are you sure all operations are added before their uses?
--------------------------------------------------------------------------------------------------
^-------------------------------------------------------------------------------------------------
| ERROR: SSAValue is not part of the IR, are you sure all operations are added before their uses?
--------------------------------------------------------------------------------------------------

@webmiche webmiche changed the title Printer: print missing ssa values instead of abort Printer: print MISSING_SSA_VALUE instead of abort Jun 15, 2022
@martin-luecke
Copy link
Collaborator

Hi @webmiche, thank you for working on this. Your current version is a very good improvement for my debugging workflow!
Does the printing abort after printing this line?

@webmiche
Copy link
Collaborator Author

webmiche commented Jun 17, 2022

Hi @webmiche, thank you for working on this. Your current version is a very good improvement for my debugging workflow! Does the printing abort after printing this line?

No, it does not. I added a testcase to reflect this behavior :)

@math-fehr
Copy link
Collaborator

Hey! Thanks a lot for working on this, this should greatly improve debugging ;)
Could you just change %MISSING_SSA_VALUE to something like <UNKNOWN>, to make sure that no one will interpret it as an ssa value, and also to make it smaller.
Unless you think there is a better reason to keep it that way?

@K-W-Li
Copy link
Collaborator

K-W-Li commented Jun 18, 2022

Last commit make a little rewrite on the logic of Printer._print_message.
Using left closed and right open notation [begin_pos, end_pos).
It gives more consistent coding and formatting, do not have to add +1 which may easily forgotten.

I code some logic to highlight the position of missing SSA value.
Now error message shows:

module() {
  %0 : !i32 = arith.addi(%MISSING_SSA_VALUE : !i32, %MISSING_SSA_VALUE : !i32)
  -----------------------^^^^^^^^^^^^^^^^^^--------------------------------------------------------
  | ERROR: SSAValue is not part of the IR, are you sure all operations are added before their uses?
  -------------------------------------------------------------------------------------------------
  --------------------------------------------------^^^^^^^^^^^^^^^^^^-----------------------------
  | ERROR: SSAValue is not part of the IR, are you sure all operations are added before their uses?
  -------------------------------------------------------------------------------------------------
  %1 : !i32 = arith.addi(%0 : !i32, %0 : !i32)
}

@math-fehr
Copy link
Collaborator

Nice! Is this ready to be merged, or do you guys want to add anything else?

@webmiche
Copy link
Collaborator Author

Waiting for you approval, and then we can merge :)

@math-fehr math-fehr merged commit 88a8241 into main Jun 23, 2022
@math-fehr math-fehr deleted the missing_ssa_val_print branch June 23, 2022 21:03
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.

Print "MISSING_SSA_VALUE" instead of aborting after exception
4 participants