On Thu, Sep 20, 2012 at 12:56 PM, Eric Botcazou <ebotca...@adacore.com> wrote:
>> I really don't like this to be done outside of SRA (and it is written in a
>> non-MEM_REF way).
>
> Could you elaborate on the latter point? If it can be improved, even in its
> current form...
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.
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).
>> For the testcase in question we scalarize back
>> 'i' in SRA (other scalars are optimized away already, but as SRA runs
>> before DSE it still gets to see stores to FRAME.i). Now I wonder
>> why we generate reasonable debug info even without inlining,
>> thus there has to be a association to the original decls with
>> the frame FIELD_DECLs. That is, lookup_decl_for_field should not be
>> necessary and what we use for debug info generation should be used by SRA
>> to assign a name to scalarized fields.
>
> The testcase is a toy example of course.
Yes, I realize that.
>> That alone would not solve your issue because of the 'arr' field in
>> the structure which cannot be scalarized (moved to a stand-alone
>> decl) by SRA. That's one missed feature of SRA though, and generally
>> useful.
>
> The improved scalarization of aggregates is the main point and what yielded
> the performance boost for SPARKSkein.
>
>> So no, I don't think this patch is the right approach.
>
> OK, but I came to the opposite conclusion when I first tried to do it in SRA
> and I don't think I will change my mind in the near future. Never mind then.
Marking the FRAME VAR_DECL looks useful, maybe you can split that out
of your patch?
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.
Maybe you can open an enhancement bugreport for this and link
your patch / testcase to it?
Thanks,
Richard.
> --
> Eric Botcazou
