-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8349835: C2: Simplify IGV property printing #26902
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
Conversation
|
👋 Welcome back snatarajan! A progress list of the required criteria for merging this PR into |
|
@sarannat This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 94 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
dafedafe
left a comment
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.
Thanks @sarannat for cleaning this up! The fix looks ok for me. I just left a couple of inline comments for minor things.
| {1, "reg_pressure", nullptr, lrg.reg_pressure()}, | ||
| {1, "cost", nullptr, (int) lrg._cost}, | ||
| {1, "area", nullptr, (int) lrg._area}, | ||
| {1, "score", nullptr, (int) lrg.score()}, |
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.
It is mainly a style matter but as _cond is a bool it might be better to use true rather than 1
| tail(PROPERTY_ELEMENT); | ||
| } | ||
|
|
||
| void IdealGraphPrinter::print_prop_record(const IdealGraphPrintRecord rec[], int size) { |
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.
Just a small naming note: the method actually prints multiple records. Could it be preferable to use the plural form?
| {((flags & Node::Flag_may_be_short_branch) != 0), "may_be_short_branch","true"}, | ||
| {((flags & Node::Flag_has_call) != 0), "has_call", "true"}, | ||
| {((flags & Node::Flag_has_swapped_edges) != 0), "has_swapped_edges", "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.
The indentation seems a bit off.
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.
BTW we might want to use TRUE_VALUE instead of "true" here.
| {(lrg._must_spill != 0), "must_spill", TRUE_VALUE}, | ||
| {(lrg._is_bound != 0), "is_bound", TRUE_VALUE}, | ||
| {lrg._msize_valid && lrg._degree_valid && lrg.lo_degree(), "trivial", TRUE_VALUE} | ||
| }; |
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.
Same intentation issue here.
|
|
||
| void IdealGraphPrinter::print_prop_record(const IdealGraphPrintRecord rec[], int size) { | ||
| for ( int i = 0; i < size; i++ ) { | ||
| if (rec[i]._cond != 0) { |
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.
As for the comment below it might be more consistent to only use rec[i]._cond as the condition.
| print_prop("trivial", TRUE_VALUE); | ||
| } | ||
| IdealGraphPrintRecord rec[] = { | ||
| {1, "mask", buffer}, |
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.
Thank for trying to clean this up! I see the benefit of doing it since it looks like we can get rid of the repetitions. But using this approach now feels like we squeezed in too much into a single generic method and fall back to arrays with structs with some optional fields that are sometimes set to null which is not easy to comprehend. And the method visit_nodes() is still quite large. What if we just had different methods or even classes for different properties? This helps with self-documenting the code and also avoids passing in some nullptr for unused values. For example (in pseudocode):
- node flag properties all share the same pattern:
class NodeFlags:
NodeFlags _flags;
print_properties() {
print_property(Flag_is_Copy, "is_copy");
print_property(Flag_rematerialize, "rematerialize");
...
}
print_property(flag, property) {
if (_flags & flag) {
print_prop(property, "true");
}
}
- For lrg: Could also create a class and store
lrgas field. We could have different printing methods, for example for unconditional prints, for printing true etc.
This is just an idea, I have not actually tried it out. What are your thoughts about that alternative approach?
We might have even more opportunities to refactor visit_node() since there are some more repetitive patterns here and there. But this could also be done separately.
jdksjolen
left a comment
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.
Hi,
I'd change the data structure to have a tagged union with separate constructors to make the struct understandable.
| struct IdealGraphPrintRecord { | ||
| bool _cond; | ||
| const char *_name; | ||
| const char *_svalue = nullptr; | ||
| int _ivalue = -1; | ||
| }; |
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.
Get rid of optionals and explicit condition checking by introducing a tagged union.
struct IdealGraphPrintRecord {
enum class State {
False, String, Integer
};
State _state;
const char* _name;
union {
const char* _svalue;
int _ivalue;
};
IdealGraphPrintRecord(bool cond, const char* name, int value)
: _state( cond ? State::Integer : State::False ), _name(name), _ivalue(value) {}
IdealGraphPrintRecord(bool cond, const char* name, const char* value)
: _state( cond ? State::String : State::False ), _name(name), _svalue(value) {}
bool has_string() { return _state == State::String; }
bool has_int() { return _state == State::Integer; }
const char* string_value() { return _svalue; }
const char* key() { ... }
int int_value() { ... }
};| const IdealGraphPrintRecord rec[] = { | ||
| {((flags & Node::Flag_is_Copy) != 0), "is_copy", "true"}, | ||
| {((flags & Node::Flag_rematerialize) != 0), "rematerialize", "true"}, | ||
| {((flags & Node::Flag_needs_anti_dependence_check) != 0), "needs_anti_dependence_check", "true"}, | ||
| {((flags & Node::Flag_is_macro) != 0), "is_macro", "true"}, | ||
| {((flags & Node::Flag_is_Con) != 0), "is_con", "true"}, | ||
| {((flags & Node::Flag_is_cisc_alternate) != 0), "is_cisc_alternate", "true"}, | ||
| {((flags & Node::Flag_is_dead_loop_safe) != 0), "is_dead_loop_safe", "true"}, | ||
| {((flags & Node::Flag_may_be_short_branch) != 0), "may_be_short_branch","true"}, | ||
| {((flags & Node::Flag_has_call) != 0), "has_call", "true"}, | ||
| {((flags & Node::Flag_has_swapped_edges) != 0), "has_swapped_edges", "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.
This code desperately needs a utility to check the corresponding bits in flags :-).
|
Thank you for the review. |
chhagedorn
left a comment
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.
Thanks for the update, that looks much better! Some more small follow-up comments.
| @@ -31,6 +31,7 @@ | |||
| #include "utilities/ostream.hpp" | |||
| #include "utilities/xmlstream.hpp" | |||
|
|
|||
|
|
|||
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.
| @@ -165,6 +169,20 @@ class IdealGraphPrinter : public CHeapObj<mtCompiler> { | |||
| void update_compiled_method(ciMethod* current_method); | |||
| }; | |||
|
|
|||
| class PrintProperties | |||
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.
Do you really need it in the header file? You could also just move it the the source file directly where we use the class.
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.
My reasoning is keep the interface and implementation separate. I have kept it this way. Will that be okay ?
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'm not sure I understand the benefit of having it separately when the only user is in the source file and it's tightly coupled to the implementation of the IdealGraphPrinter class. This will expose it to other files while it's not needed. Or is it just for readability?
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.
Yes, it was mostly for readability. I do agree with you and have now followed your suggestion of moving the class to the source file.
| @@ -1220,6 +1111,76 @@ void IdealGraphPrinter::update_compiled_method(ciMethod* current_method) { | |||
| } | |||
| } | |||
|
|
|||
| void PrintProperties::print_node_properties(Node* node, Compile* C){ | |||
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.
| void PrintProperties::print_node_properties(Node* node, Compile* C){ | |
| void PrintProperties::print_node_properties(Node* node, Compile* C) { |
| if (flag) { | ||
| _printer->print_prop(name, IdealGraphPrinter::TRUE_VALUE); | ||
| } | ||
| } | ||
|
|
||
| void PrintProperties::print_property(int flag, const char* name, const char* val) { | ||
| if (flag) { | ||
| _printer->print_prop(name, val); | ||
| } | ||
| } | ||
|
|
||
| void PrintProperties::print_property(int flag, const char* name, int val) { | ||
| if (flag) { | ||
| _printer->print_prop(name, val); | ||
| } |
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.
We should not use implicit conversion of ints, same below:
| if (flag) { | |
| _printer->print_prop(name, IdealGraphPrinter::TRUE_VALUE); | |
| } | |
| } | |
| void PrintProperties::print_property(int flag, const char* name, const char* val) { | |
| if (flag) { | |
| _printer->print_prop(name, val); | |
| } | |
| } | |
| void PrintProperties::print_property(int flag, const char* name, int val) { | |
| if (flag) { | |
| _printer->print_prop(name, val); | |
| } | |
| if (flag != 0) { | |
| _printer->print_prop(name, IdealGraphPrinter::TRUE_VALUE); | |
| } | |
| } | |
| void PrintProperties::print_property(int flag, const char* name, const char* val) { | |
| if (flag != 0) { | |
| _printer->print_prop(name, val); | |
| } | |
| } | |
| void PrintProperties::print_property(int flag, const char* name, int val) { | |
| if (flag != 0) { | |
| _printer->print_prop(name, val); | |
| } |
| class PrintProperties | ||
| { | ||
| private: | ||
| IdealGraphPrinter *_printer; |
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.
For new code, we should put the the * at the type.
| public: | ||
| PrintProperties(IdealGraphPrinter *printer) : _printer(printer) {} | ||
| void print_node_properties(Node *node, Compile *C); | ||
| void print_lrg_properties(const LRG &lrg, const char *buffer); |
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.
is passing by reference done to avoid copying?
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.
That is my only reason for doing this.
| print_property(!(C->matcher()->is_shared(node)), "is_shared", IdealGraphPrinter::FALSE_VALUE); | ||
| print_property(C->matcher()->is_dontcare(node), "is_dontcare"); | ||
| print_property(!(C->matcher()->is_dontcare(node)),"is_dontcare", IdealGraphPrinter::FALSE_VALUE); | ||
| print_property((C->matcher()->find_old_node(node) != nullptr), "old_node_idx", C->matcher()->find_old_node(node)->_idx); |
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 we might have an issue here: C->matcher()->find_old_node(node)->_idx is always evaluated no matter if C->matcher()->find_old_node(node) != nullptr or not.
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.
Yes, I have fixed this now.
robcasloz
left a comment
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.
Thanks for working on this, Saranya! We do not have good test coverage of IGV graph dumping, so only running regular tier testing might not catch possible regressions introduced by this changeset. Consider also comparing the XML graphs dumped before and after the changeset for a few well-known methods with deterministic compilation, e.g.:
$ ${BASELINE_JAVA} -Xbatch -XX:PrintIdealGraphLevel=6 -XX:CompileOnly=java.lang.String::charAt -XX:PrintIdealGraphFile=before.xml
$ ${PATCHED_JAVA} -Xbatch -XX:PrintIdealGraphLevel=6 -XX:CompileOnly=java.lang.String::charAt -XX:PrintIdealGraphFile=after.xml
$ diff before.xml after.xml
The only expected changes between the two files would be things like memory addresses, process and thread IDs, etc. But all the other properties should remain the same.
|
@sarannat this pull request can not be integrated into git checkout JDK-8349835
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
@robcasloz: I followed the testing methodology you suggested for the below commands. I did hit a crash that I fixed, after that the only difference were to the memory, process, and thread ID. In this testing, I did NOT see prints for |
robcasloz
left a comment
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.
Thanks for cleaning up this code and testing it thoroughly, Saranya. The changes look good to me overall, I just have a few minor suggestions.
There may be more properties that could be printed using the new abstraction, but I am OK with addressing those separately.
| class InlineTree; | ||
| class ciMethod; | ||
| class JVMState; | ||
| class LRG; |
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.
This forward declaration is not needed anymore.
| print_property(!(C->matcher()->is_dontcare(node)),"is_dontcare", IdealGraphPrinter::FALSE_VALUE); | ||
| Node* old = C->matcher()->find_old_node(node); | ||
| if (old != nullptr) { | ||
| print_property(true, "old_node_idx", C->matcher()->find_old_node(node)->_idx); |
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.
| print_property(true, "old_node_idx", C->matcher()->find_old_node(node)->_idx); | |
| print_property(true, "old_node_idx", old->_idx); |
| public: | ||
| PrintProperties(IdealGraphPrinter* printer) : _printer(printer) {} | ||
| void print_node_properties(Node* node, Compile* C); | ||
| void print_lrg_properties(const LRG &lrg, const char* buffer); |
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.
| void print_lrg_properties(const LRG &lrg, const char* buffer); | |
| void print_lrg_properties(const LRG& lrg, const char* buffer); |
|
|
||
| public: | ||
| PrintProperties(IdealGraphPrinter* printer) : _printer(printer) {} | ||
| void print_node_properties(Node* node, Compile* C); |
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 suggest to fetch the Compile reference within print_node_properties from _printer->C instead.
| void print_node_properties(Node* node, Compile* C); | |
| void print_node_properties(Node* node); |
chhagedorn
left a comment
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.
Thanks for the updates! Apart from Roberto's suggestions, it looks good!
|
|
|
Thank you for the review @robcasloz. I have now addressed these and filed JDK-8372273 to address the rest of the node properties. |
robcasloz
left a comment
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.
Thanks for addressing my comments Saranya, I just have one final suggestion, looks good otherwise.
| if (_printer->C->matcher() != nullptr) { | ||
| print_property(_printer->C->matcher()->is_shared(node),"is_shared"); | ||
| print_property(!(_printer->C->matcher()->is_shared(node)), "is_shared", IdealGraphPrinter::FALSE_VALUE); | ||
| print_property(_printer->C->matcher()->is_dontcare(node), "is_dontcare"); | ||
| print_property(!(_printer->C->matcher()->is_dontcare(node)),"is_dontcare", IdealGraphPrinter::FALSE_VALUE); | ||
| Node* old = _printer->C->matcher()->find_old_node(node); |
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.
| if (_printer->C->matcher() != nullptr) { | |
| print_property(_printer->C->matcher()->is_shared(node),"is_shared"); | |
| print_property(!(_printer->C->matcher()->is_shared(node)), "is_shared", IdealGraphPrinter::FALSE_VALUE); | |
| print_property(_printer->C->matcher()->is_dontcare(node), "is_dontcare"); | |
| print_property(!(_printer->C->matcher()->is_dontcare(node)),"is_dontcare", IdealGraphPrinter::FALSE_VALUE); | |
| Node* old = _printer->C->matcher()->find_old_node(node); | |
| Matcher* matcher = _printer->C->matcher(); | |
| if (matcher != nullptr) { | |
| print_property(matcher->is_shared(node),"is_shared"); | |
| print_property(!matcher->is_shared(node), "is_shared", IdealGraphPrinter::FALSE_VALUE); | |
| print_property(matcher->is_dontcare(node), "is_dontcare"); | |
| print_property(!matcher->is_dontcare(node),"is_dontcare", IdealGraphPrinter::FALSE_VALUE); | |
| Node* old = matcher->find_old_node(node); |
dafedafe
left a comment
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.
Thanks again @sarannat. I just have one more nit/doubt. Looks good to me otherwise.
| void PrintProperties::print_lrg_properties(const LRG &lrg, const char *buffer) { | ||
| print_property(true, "mask", buffer); | ||
| print_property(true, "mask_size", lrg.mask_size()); | ||
| if (lrg._degree_valid) { |
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 know it was there already, but do we still need this if?
|
/integrate |
|
Going to push as commit 5fe731d.
Your commit was automatically rebased without conflicts. |
Issue
The code that prints node properties and live range properties is very verbose and repetitive and could be simplified by applying a refactoring suggested here.
Fix
Implemented the suggested refactoring.
Testing
Github Actions, Tier 1-3
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26902/head:pull/26902$ git checkout pull/26902Update a local copy of the PR:
$ git checkout pull/26902$ git pull https://git.openjdk.org/jdk.git pull/26902/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26902View PR using the GUI difftool:
$ git pr show -t 26902Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26902.diff
Using Webrev
Link to Webrev Comment