On Fri, Sep 21, 2012 at 12:48 PM, Eric Botcazou <ebotca...@adacore.com> wrote:
>> You rely on being able to see all FRAME accesses as component refs,
>> thus nothing transforms them into just MEM[&FRAME, offset]. That's of
>> course something that can be easily "broken" by means of doing
>> some pointer arithmetic like (untested, but you get the idea)
>>
>> foo()
>> {
>> int c[32];
>> int j;
>> bar()
>> {
>> int *p = &c[4];
>> p = p + 1;
>> j = *p;
>> }
>> c[4] = 0;
>> bar();
>> return j;
>> }
>>
>> this should get you j = MEM[<CHAIN>, 4]; in bar and thus a missing
>> component-ref when inlining.
>
> The patch compiles hundreds of thousands of lines of Ada everyday at AdaCore,
> how could such a blatant hole have survived that?
>
>> I dont' think it's easily possible to recover from this in your scheme,
>> but it would be straight-forward for SRA (you basically look for the
>> base variable FRAME and special-case that completely for
>> replacement construction, also constraining accesses).
>
> Well, it's implemented in the 30-line block of code under the comment:
>
> + /* Deal with remaining MEM_REFs, i.e. those for which the field reference
> + has been replaced with the offset. */
>
>> Marking the FRAME VAR_DECL looks useful, maybe you can split that out
>> of your patch?
>
> Sure.
>
>> As of doing it in SRA what I'd do there is special-case FRAME for both
>> candidate consideration (so you get around the addressable issue)
>> and replacement generation.
>
> OK, but you need to be able to split the FRAME structure without necessarily
> splitting its aggregate fields. Is that (easily) doable with current SRA?
In theory yes. Though I'd start with trying to improve general SRA here
(because I think it doesn't do that yet), like for
struct { int i; struct { int j0; int j1; .... int j128; } s; } x;
while it can scalarize x.i it won't pull out x.s (similar for arrays). Usually
addressability prevents that from being a valid transform, but excessive
size of sub-structures can prevent SRA from working on them as well.
The replacement machinery at least should handle it fine (not the
replacement object geneation - that only generates SSA names currently).
Likewise SRA can be extended to handle
struct { int i; int a[32]; } s;
with variable index accesses to a by that means (the accesses should
already be properly constrained - what is missing is a pass over all
structure fields matching them with accesses to see if aggregate
replacements are possible).
Yes, most of the SRA heuristic games make it complicated and ugly,
especially as it is isn't clearly separate analysis / decision / transform
phases. TLC welcome ;)
>> Maybe you can open an enhancement bugreport for this and link
>> your patch / testcase to it?
>
> Will do.
Thanks,
Richard.
> --
> Eric Botcazou
