This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Further insn-attrtab.c speedup
On Mon, Jun 21, 2010 at 02:08:36PM +0200, Michael Matz wrote:
> There's quite some complexity you had to add to handle such situations
> if (cond1 && get_attr_x () ...)
> else if (cond2 && get_attr_x () ...)
> Here the first get_attr_x call doesn't dominate the second one, hence you
> can't transform this sequence into just one call using the cached value.
Yeah, the patch figures that out and doesn't use the cached_x ==
variant in the second if.
But e.g. for
if (cond1 || get_attr_x () == A)
else if (cond2 || get_attr_x () == B)
it would. Or for
if ((cond1 && get_attr_x () == A) || cond2)
if (get_attr_x () == B)
> But backends must be able to determine all attributes for all
> instructions, so we _can_ transform this into
> cached_x = get_attr_x ();
> if (cond1 && cached_x ...)
> else if (cond2 && cached_x ...)
> Which is what my patches were doing, getting rid of all get_attr_... calls
> inside conditions.
The get_attr_x () functions are often quite expensive, so I didn't want to
introduce calls that would return values that wouldn't be used.
Yeah, the generated code could be tiny bit smaller, but I'd be worried that
it would be slower too.
> I'm especially worried about a sequence of get_attr()
> calls in conditions where you can remove only the first one, if at all.
> This would mean to evaluate some get_attr calls more often than in an
> unmodified insn-attrtab, but in past measurements this didn't change
> performance (and reduced code size even more).
My patch doesn't add any extra get_attr_* calls. All it does is transform
some (get_attr_X (insn) == something) expressions to
((cached_X = get_attr_X (insn)) == something)
and some into
(cached_X == something).
> > Ok to commit (assuming the speedup patch is approved;
> Mark approved your first speedup patch.
I wasn't sure if Mark's mail was meant as an ack, anyway, Richard acked it