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

[Vladimir Kozlov]

On Wed, 23 Feb 2022 14:17:50 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> 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.

I agree with @JornVernee  here.

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

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