On Wed, Feb 12, 2014 at 6:51 PM, Eric Botcazou <ebotca...@adacore.com> wrote:
>> I am not sure how to deal with this, given that we have mismatched
>> V_C_Es anyway, I'm inclined not to care and let the expander deal with
>> it. But at the same I understand that it is ugly and will certainly
>> cause somebody more headache in the future. I suppose that not
>> scalarizing here might hurt performance and would be frowned upon at
>> the very least. If the fields bigger than the record approach is the
>> standard way of doing this, perhaps SRA can detect such cases and
>> produce these strange COMPONENT_REFs instead, but is it so?
>
> You may remember that we went that way before (building a COMPONENT_REF for
> bit-fields instead of fully lowering the access) so doing it again would be a
> step backwards. Likewise if we refuses to scalarize. So IMO it's either low-
> level fiddling in SRA or in the expander (my preference too).
Ok, I've looked at the testcase and I suppose the following change is
what triggers the bug:
<bb 11>:
_56 = m.P_ARRAY;
- my_rec2.r1 = VIEW_CONVERT_EXPR<struct opt31__rec1>(*_56[1 ...]{lb:
_3 sz: 1});
- _58 = my_rec2.r1.f;
+ _51 = VIEW_CONVERT_EXPR<opt31__time_t___XDLU_0__11059199>(*_56[1
...]{lb: _3 sz: 1});
+ my_rec2$r1$f_43 = _51;
+ _58 = my_rec2$r1$f_43;
if (_58 > 11059199)
I observe that SRA modifies an existing but not replaced memory reference
(something I always thought is asking for trouble). It changes
VIEW_CONVERT_EXPR<struct opt31__rec1>(*_56[1 ...]{lb: _3 sz: 1});
to VIEW_CONVERT_EXPR<opt31__time_t___XDLU_0__11059199>(*_56[1 ...]{lb:
_3 sz: 1});.
Created a replacement for my_rec2 offset: 128, size: 24: my_rec2$r1$f
Access trees for my_rec2 (UID: 2659):
access { base = (2659)'my_rec2', offset = 128, size = 24, expr =
my_rec2.r1.f, type = opt31__time_t___XDLU_0__11059199, grp_read = 1,
grp_write = 1, grp_assignment_read = 1, grp_assignment_write = 1,
grp_scalar_read = 1, grp_scalar_write = 0, grp_total_scalarization =
0, grp_hint = 0, grp_covered = 1, grp_unscalarizable_region = 0,
grp_unscalarized_data = 0, grp_partial_lhs = 0, grp_to_be_replaced =
1, grp_to_be_debug_replaced = 0, grp_maybe_modified = 0,
grp_not_necessarilly_dereferenced = 0
but obviously 'type' doesn't agree with 'size' here.
In other places we disqualify exprs using VIEW_CONVERT_EXPRs but
appearantly only for the candidate itself, not for stuff assigned to it.
(though I never understood why disqualifying was necessary at all
for VIEW_CONVERT_EXPRs).
We are using the type of a bitfield field for the replacement which
we IMHO should avoid because the FIELD_DECLs size is 24
but the fields type TYPE_SIZE is 32 (it's precision is 24). That's
all not an issue until you start to VIEW_CONVERT to such type
(VIEW_CONVERT being a reference op just cares for size not
precision). Other ops are treated correctly by expansion.
Now - using a non-mode precision integer type as scalar replacement
isn't going to produce great code and, as we can see, has "issues"
when using VIEW_CONVERT_EXPRs.
SRA should either avoid this transform or fixup by VIEW_CONVERTing
memory reads only to mode-precision integer types and then inserting
a fixup cast. The direct VIEW_CONVERsion it creates, from
my_rec2.r1 = VIEW_CONVERT_EXPR<struct opt31__rec1>(*_56[1 ...]{lb: _3 sz: 1});
_58 = my_rec2.r1.f;
to basically
_58 = VIEW_CONVERT_EXPR<opt31__time_t___XDLU_0__11059199>(*_56[1
...]{lb: _3 sz: 1});
is simply wrong.
If you "fix" expansion then consider a nested VIEW_CONVERT_EXPR
that views back to the aggregate type - is that now supposed to
clear the upper 8 bits because of the VIEW_CONVERT_EXPR in the
middle? Not so. So fixing VIEW_CONVERT_EXPR sounds conceptually
wrong to me.
Not scalarizing a field to a DECL_BIT_FIELD FIELD_DECLs type looks like
the best fix to me.
Richard.
