RFR: 8281548: Add escape analysis tracing flag [v4]

[Jorn Vernee]

On Wed, 16 Feb 2022 01:54:28 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> I can get the debugger argument, but I'm not sure I see how doing this would make the code cleaner...
>> 
>> Instead of printing out the reason message directly in set_(fields_)escape_state/set_not_scalar_replaceable we would set it in `PointsToNode.reason`, and then afterwards walk the graph to print out all messages?
>> 
>> One potential problem with that approach is that if a node is first updated to ArgEscape, and then later to GlobalEscape, the first state transition would not be visible. (i.e. the PR is not just annotating nodes, it's annotating state transitions, and there can be multiple state transitions per node)
>
> C/C++ macro is very powerful. however, it may scramble code.  If we refrain abusing, I think C++ can achieve cleaner code like Java. 
> 
>> Instead of printing out the reason message directly in set_(fields_)escape_state/set_not_scalar_replaceable we would set it in PointsToNode.reason, and then afterwards walk the graph to print out all messages?
> 
> I mean we can do thing like this: 
> ```ptn->reason = "stored at raw address";```
> 
> and then 
> 
> void ConnectionGraph::trace_es_update_helper(PointsToNode* ptn, PointsToNode::EscapeState es, bool fields) const {
>   if (_compile->directive()->TraceEscapeAnalysisOption) {
>     assert(ptn != nullptr, "should not be null");
>     assert(ptn->reason != nullptr, "should not be null");
>     ptn->dump_header(true);
>     PointsToNode::EscapeState new_es = fields ? ptn->escape_state() : es;
>     PointsToNode::EscapeState new_fields_es = fields ? es : ptn->fields_escape_state();
>     tty->print_cr("-> %s(%s) %s", esc_names[(int)new_es], esc_names[(int)new_fields_es], ptn->reason);
>   }
> }
> 
> 
> PointsToNode::reason is just a global variable. it can prevent you from passing the argument under macro expansion. 
> if inliner and scalar replacement of gcc is at work, this field can be removed. Since we only use PointsToNode::reason in debug build, it doesn't matter if gcc misses that.

Ok, so if I understand correctly, the issue is with the `NOT_PRODUCT(COMMA ...)` in the arguments list.

I think it doesn't look great either, but it seems like the best alternative if we want to make sure no code is generated in product builds. I think using macros in this way is the best way of doing that. If we were to depend on compiler optimizations to eliminate bits in product builds, I think we should just make the reason parameter unconditional, rather than piggybacking on a field of PointsToNode. The compiler should see that it is unused on product builds. That seems even cleaner. But I think the intent is much clearer with the NOT_PRODUCT macro, since it's more obvious that this code will not be in product builds from the call site.

I don't like the idea of splitting the reason string out of the parameter list either, since it requires callers to remember to also separately set the reason string. Having it as a parameter forces callers to think about it.

An alternative idea is to declare new macros for setting the escape state and scalar replaceability like so:

    #ifndef PRODUCT
    #define SET_ESCAPE_STATE(node, es, reason) set_escape_sate(node, es, reason)
    #else 
    #define SET_ESCAPE_STATE(node, es, reason) set_escape_sate(node, es)
    #endif

Where the product version drops the reason string. But, I think this is also a bit too magic, and a bit worse than the current solution.

Sorry, but I think I'd rather stick with what I have now.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7428