This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix PR middle-end/17746


[Thanks again for looking into this.]

> Again, my question is: what behavior do we expect from this code?
> Given the quoted code
>
>     ADDR_EXPR
>       COMPONENT_REF
>         VIEW_CONVERT_EXPR <record_type1 ... align-ok>
>           INDIRECT_REF <record_type2>
>             VAR_DECL <pointer_type <record_type2>>
>
> where the VIEW_CONVERT_EXPR is increasing the alignment, either
> 1) we know that the pointer already has the proper alignment, or
> 2) the pointer might not have the proper alignment, so we might need to
>    copy into a record_type1 temporary with the appropriate alignment.

I think it's 1).  My understanding is that the polymorphic pointer is 
guaranteed to be sufficiently aligned by the front-end through the setting of 
the TYPE_ALIGN_OK flag on the VIEW_CONVERT_EXPR.

> I suspect that the get_inner_reference change was needed because #2 is
> true.  In that case, the gimplifier should allocate a temporary for the
> VIEW_CONVERT_EXPR on platforms where a temporary is needed.

Reading the audit trail for TN BA08-006, it is clear that the change was made 
to *prevent* a temporary from being allocated.  A few excerpts:

"I'll take a look, if nothing else to understand the semantics here! This is
 a protected object, so it's certainly wrong to copy it."

"The alignments are fine, the discrepancy is expected, this should work. I
  rather suspect that the mechanism for the Object that is the implicit
 parameter of a protected operation is not labelled as By_Reference, and
 the back-end may decide that it has the permission to pass it by
 copy. Let me suggest a patch to try on gnat5. If this doesn't work, it's
 a deeper back-end problem."

Richard's final comment:
"Ed and I worked on this and it turns out that because of the tagged
 types, the alignment is OK and the appropriate flags were set.  But
 because of the use of a component of the type, they were ignored.  I
 have a fix for it."


In light of this, my interpretation is that Richard's strategy was to force 
get_inner_reference to "temporarily" exit, so that the VIEW_CONVERT_EXPR 
expander can take control and step up the alignment:

    case VIEW_CONVERT_EXPR:
      op0 = expand_expr (TREE_OPERAND (exp, 0), NULL_RTX, mode, modifier);

      [...]

      if (MEM_P (op0))
	{
	  op0 = copy_rtx (op0);

	  if (TYPE_ALIGN_OK (type))
	    set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));


To sum up, I think the gimplifier must not allocate a temporary for 
VIEW_CONVERT_EXPR in the TYPE_ALIGN_OK case, but could do so in other 
cases.  However, only its expander currently knows the exact condition.


About the handled_component_p/get_inner_reference discrepancy: a widely used 
idiom is

  while (handled_component_p (t))
    t = TREE_OPERAND (t, 0);

So if we made handled_component_p reject VIEW_CONVERT_EXPRs with the 
TYPE_ALIGN_OK flag, I suspect we would probably need to add explicit

  || ((TREE_CODE (t) == VIEW_CONVERT_EXPR && TYPE_ALIGN_OK (t) && ...)

all over the place.

> Incidentally, the expand_expr_real/VIEW_CONVERT_EXPR code doesn't seem to
> try to use 'target' in the alignment mismatch case; it seems to me that it
> should do that before it allocates a temporary.

Indeed, that could be a useful improvement.

-- 
Eric Botcazou


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]