Re: [PATCH 6/6] Make SRA replace constant-pool loads

[Alan Lawrence <alan dot lawrence at arm dot com>]

On 3 November 2015 at 14:01, Richard Biener <richard.guenther@gmail.com> wrote:
>
> Hum.  I still wonder why we need all this complication ...

Well, certainly I'd love to make it simpler, and if the complication
is because I've gone about trying to deal with especially Ada in the
wrong way...

>  I would
> expect that if
> we simply decided to completely scalarize a constant pool aggregate load then
> we can always do that.  SRA should then simply emit the _same_ IL as it does
> for other complete scalarization element accesses.  The only difference is
> that the result should be simplifiable to omit the element load from
> the constant pool.
>
> 1) The FRE pass following SRA should do that for you.
...
> That is, I'd like to see the patch greatly simplified to just consider
> constant pool
> complete scalarization as if it were a regular variable.

Hmm, can you clarify, do you mean I should *not* replace constant pool
values with their DECL_INITIAL? The attempt to substitute in the
initial value is what leads to most of the problems. For example, in
gnat/opt31.adb, create_access finds this expression accessing *.LC0:

MEM[(interfaces__unsigned_8[(sizetype) <PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->LB0:<PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->UB0 >= <PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->LB0 ? (sizetype) <PLACEHOLDER_EXPR
struct opt31__messages_t___XUP>.P_BOUNDS->UB0 : (sizetype)
<PLACEHOLDER_EXPR struct opt31__messages_t___XUP>.P_BOUNDS->LB0 +
4294967295] *)&*.LC0][1 ...]{lb: 1 sz: 1}

this is an ARRAY_RANGE_REF of a MEM_REF of an ADDR_EXPR of *.LC0. So
far I haven't extended subst_constant_pool_initial to handle
ARRAY_RANGE_REFs, as it can't even handle this MEM_REF:

MEM[(interfaces__unsigned_8[(sizetype) <PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->LB0:<PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->UB0 >= <PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->LB0 ? (sizetype) <PLACEHOLDER_EXPR
struct opt31__messages_t___XUP>.P_BOUNDS->UB0 : (sizetype)
<PLACEHOLDER_EXPR struct opt31__messages_t___XUP>.P_BOUNDS->LB0 +
4294967295] *)&*.LC0]

because the type here has size:

MIN_EXPR <_GLOBAL.SZ2.ada_opt31 (<PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->UB0, <PLACEHOLDER_EXPR struct
opt31__messages_t___XUP>.P_BOUNDS->LB0), 17179869176>

inside the MEM_REF of the ADDR_EXPR is *.LC0, whose DECL_INITIAL is a
4-element array (fine). Sadly while the MEM_REF
type_contains_placeholder_p, the type of the outer ARRAY_RANGE_REF
does not....

One possibility is that this whole construct, ARRAY_RANGE_REF that it
is, should mark *.LC0 in cannot_scalarize_away_bitmap.

However, disqualified_constants is also necessary if I want a
meaningful assertion that we do not re-add constant pool entries as
candidates after we've discovered them - or should I try to rely on
there only being one expression that accesses each constant pool
entry? (I don't think that's guaranteed as tree_output_constant_def
does hashing, I admit I haven't really tried to break that assumption)

> 2) You should be able to use fold_ctor_reference directly (in place of
> all your code
> in case offset and size are readily available - don't remember exactly how
> complete scalarization "walks" elements).  Alternatively use
> fold_const_aggregate_ref.

That should work for completely_scalarize, yes, i.e. if I can remove
the other route in create_access.

> 3) You can simplify the stmt SRA generated by simply calling fold_stmt on it,
> that will do a bit more (wasted) work compared to 2) but may be easier.
>
> I wouldn't bother with the case where we for some reason do not simplify
> the constant pool load.

Hmmm. As above, I'm not quite sure what you mean by "the constant pool
load" - if that means, substituting the DECL_INITIAL in place of the
VAR_DECL that is DECL_IN_CONSTANT_POOL, then I didn't find a general
tree substitution routine, hence writing subst_constant_pool_initial.
It might be possible to make that simpler/more generic (e.g. just call
copy_node, recurse on each operand, return the original if nothing
changed) and then fold at the end.

Thanks,
Alan