Re: [patch] Fix wrong code with VCE to bit-field type at -O

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Feb 12, 2014 at 6:51 PM, Eric Botcazou <ebotcazou@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.