-
Notifications
You must be signed in to change notification settings - Fork 444
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
Call PassManager's debug hook even after a failing pass #4626
Conversation
if (node) { | ||
node->apply(toP4); | ||
} else { | ||
*stream << "No P4 program returned by the pass" << std::endl; |
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.
So this only happens when there is an error right? I would clarify this in the message.
ir/pass_manager.cpp
Outdated
if (stop_on_error && ::errorCount() > initial_error_count) break; | ||
if ((program = after) == nullptr) break; | ||
if (stop_on_error && ::errorCount() > initial_error_count) early_exit_flag = true; | ||
if ((program = after) == nullptr) early_exit_flag = true; |
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.
What does it mean when (program = after) == nullptr
? Unclear to me.
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 think this is C/C++'s fun "you can embed assignment statements inside of expressions" thing, semantically equivalent to doing:
program = after;
if (program == nullptr) early_exit_flag = true;
Or maybe I don't understand your question.
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.
Oh yes, it was more meant as a clarification question under which circumstances the after
is nullptr.
But that expression should also be simplified while we are at it.
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.
At least the Transform's code suggests it is possible to return nullptr
in the pre/postorder and if you do that for root the entire apply
would yield nullptr
. This would be quite weird I guess, but it is allowed.
I've pushed a commit that eliminates the after
variable altogether.
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.
Approving, although I can not say for sure there won't be any unintended side-effects.
One effect I can see is that the compiler will attempt to dump P4 also form the pass that triggered an error. That would of course happen only if dumping is enabled, so in develompment. The rest will depend on the debug hooks used by downstream projects, which is hard to know... If this turns out to be problematic, the hook can always check |
breaking change: Debug hooks in
PassManager
can now be called with the IR::Node beingnullptr
. This indicates that the last executed pass failed and did not return any value.This allows running a debug hook also after a failing pass too. This can be useful e.g. to check diagnostic counts for the pass in the debug hook.