Re: PATCH RFA: PR 29286: Handle placement new aliasing issues

[Michael Matz <matz at suse dot de>]

Hi,

On Mon, 10 Jun 2007, Ian Lance Taylor wrote:

> It won't offend me if we don't use my patch.
> 
> But Richard's proposed rules will hurt optimization in some cases. They 
> didn't hurt his particular test case.

Well, his testcase actually was SPEC2000, polyhedron and a couple of heavy 
and not so heavy C++ programs.

> But they will certainly hurt other test cases, as discussed in the 
> further comments to the PR, e.g., comments 127 (partially incorrect) and 
> 140.

All of those just speculate that it _might_ influence performance badly.  
Noone in the bugreport actually showed some.  And Richard just showed the 
opposite.  To be perfectly clear for readers of this thread who don't 
like reading the whole PR, the rule would ensure that we

* don't reorder stores
* don't sink loads over stores

I.e. the "natural" movements (when seen from a loop perspective) are still 
allowed (hoisting loads, also over stores; and sink stores, also over 
loads).

People didn't like such rule because of the perceived limitation of 
optimizers, without showing any evidence.  It of course is a limitation, 
but not necessarily an important one.  Apart from that it should be noted 
that currently our whole tree-ssa framework reorders stores only at one 
occasion, and there it's by accident (tree-ssa-loop-im.c), so that can't 
be important optimization.  It would probably be enlightening if someone 
would count how often we sink loads over stores (or hoist stores over 
loads); I have no data, but my guess is not often.  If that is true then 
the point about optimizer limitation is mood.

> So I don't think they are a good idea for that reason.  And I also think 
> it is a very bad idea to have rules which are not explicitly represented 
> in the IR.

That is indeed a problem.  When I discussed this with Richard I came up 
with something, but that would be extremely ugly and wasteful.  
Essentially you want to ensure that loads are not moved below stores.  So 
there needs to be a dependency from stores to loads, which we 
traditionally express as vops, so every load would "define" some special 
vop, which stores would then vuse.  The operand scanner would have to take 
care to insert all the right symbols, the ssa renamer would take care of 
building the chains, and after optimization passes (but before rebuilding 
SSA form for those vops) one could check that no invalid move was done by 
any transformation.

But as said, that's ugly and unintuitive.  When thinking longer about it I 
more or less convinved myself that it's actually really easier to just 
look at all transformations, see if they do something interesting with 
stores or loads, and check them if they follow the rules.

> Also, I disagree that my proposed patch is fragile.

Fragile was the wrong word.  My gut feeling is, that the patch solves a 
very special problem at a very touchy point in the compiler (the aliaser), 
when we _could_ solve a more general (to some only potential) problem with 
less effort, and without many disadvantages.

> The semantics of CHANGE_DYNAMIC_TYPE_EXPR are reasonably clear.  And the 
> aliasing effects are reasonably clear.  The alias analysis code for TBAA 
> is encapsulated precisely in the function may_alias_p--there is even a 
> statistical counter for it.  The only potential fragility I see is the 
> handling of no_tbaa_pruning in the solver.

Hmm, please don't get me wrong with what I write now, but you needed more 
than one month and seven attempts to get this thing working, and still 
find everything reasonably clear?  I mean, if _you_ needed a month for it, 
what do you think how long it will take the avarage hacker to wrap his 
brain around it? ;-/


Ciao,
Michael.