Re: [PATCH 5/5] perf metric: Don't compute unused events.

[Andi Kleen <ak@linux.intel.com>]

The patch does a lot of stuff and is hard to review.

The earlier patches all look good to me.

>  
>  static int
>  __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
> -	      int start, int runtime)
> +	      bool compute_ids, int runtime)
>  {
>  	struct expr_scanner_ctx scanner_ctx = {
> -		.start_token = start,

Okay so why do we not need that anymore?

This should probably be a separate change.

I'm not sure about the NaN handling. Why can't you use some 
other value (perhaps 0 with some special case for division) 

After all the computed values of the dead branch shouldn't 
be used anyways, so I don't think we need the extra NaN 
ceremony everywhere.

The only thing that really matters is to track that the event
is not added and not resolved.

I think it would be a lot simpler if we changed the IF syntax.
The problem right now is that the condition is after the first
expression, so you can't use global state with an early reduction
to track if the if branch is dead or not.

If we used the C style cond ? a : b  it could be

cond { set flag } '?' expr  { reset flag } : expr { reset flag }

scanner checks flag and ignores the event if false

This would need changing the JSON files but that should be easy enough
with a script.

Or maybe at some point need to bite the bullet and build
an AST, but for now we probably can get away of not doing it.


-Andi