Re: [Valgrind-developers] dirty helpers with guards in memcheck (patch set)

[Florian K. <br...@ac...>]

Thanks for looking over the patches..

On 07/10/2012 11:45 AM, Julian Seward wrote:
> 
> It'd be reassuring to test some completely deterministic app on x86-64
> (eg, perf/bz2.c compiled -O2) to check that the quantity of JIT generated
> code is unchanged.
> 

I did some measurements with a modified patch that incorporates all your
suggested changes. Runtime is unchanged.
Instruction measurements show no significant detriment:

trunk unmodified
bz2     transtab: new  3,698 (99,519 -> 1,566,490; ratio 157:10)
fbench  transtab: new  2,336 (55,102 -> 877,541; ratio 159:10)
ffbench transtab: new  2,115 (51,832 -> 828,083; ratio 159:10)
heap    transtab: new  1,846 (40,908 -> 663,257; ratio 162:10)
tinycc  transtab: new  4,662 (119,656 -> 1,926,502; ratio 161:10)

trunk with patches
bz2     transtab: new  3,698 (99,519 -> 1,566,490; ratio 157:10)
fbench  transtab: new  2,336 (55,102 -> 877,534; ratio 159:10)
ffbench transtab: new  2,115 (51,832 -> 828,090; ratio 159:10)
heap    transtab: new  1,846 (40,903 -> 663,264; ratio 162:10)
tinycc  transtab: new  4,662 (119,646 -> 1,926,509; ratio 161:10)

Virtually identical insn counts.

> Looks plausible.  Couple of things:
> 
> (trivial) 
> -
>  /*------------------------------------------------------------*/
> 
> pls can you leave the blank line there?  I like 2 blank lines
> before section breaks (more artistically pleasing :)

Yes, sure. This was unintentional. I share your artistic preference.

> +   /* If the complaint is to be issued under a guard condition, AND that
> +      guard condition. */
> +   if (guard && ! isTRUE(di->guard)) {
> 

...snip ...

> That said, I would much prefer that you removed the " && ! isTRUE(di->guard)",
> hence always constructing the And against di->guard if it is non-NULL, and
> rely in ir_opt to tidy up afterwards if di->guard is a constant. 

Ok. No problem. Above insn numbers show that your confidence in the
optimisers is warranted.

> +   We assume here, that the validity of GUARD has already been checked.
> s/validity/definedness

Yep.

> I don't think that iropt can remove "PUT(offs) = GET(offs)".  Not 100%
> sure it can't though.  Doesn't affect correctness.

Throwing those out should be allowed for those guests offsets that do
not require precise memory exceptions. I did not check whether
redundant_put_removal actually throws them out.

> Presumably there is some way in all this that MC will complain if the
> guard expression is undefined.  Maybe the existing code already does that?

Yes, the existing code already checks the definedness of the guard
expression.

>> patch-5 modifies the amd64 insn selector which would otherwise assert.
> 
> +      addInstr(env, AMD64Instr_Test64(0xFFFFFFFF,reg));
> 
> Shouldn't that be 0x1 ?  IIUC, as you have it, rflags.NZ will get set to 1
> if any of the bits in reg are nonzero.

I wasn't sure about this one. I take your word and change it.

I have an updated patch which I will post when s390 passes. There is
currently one testcase that is failing and I need to find out why. Could
be some latent bug that is exposed by these changes.

   Florian