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: [tree-ssa] fix latent & real bugs in BIT_FIELD_REFs


On Wed, 2004-03-24 at 11:04, Paolo Bonzini wrote:

> Nope.  You can get a BIT_FIELD_REF from add_stmt_operand when fold tries to 
> put together separate COMPONENT_REFs into a single BIT_FIELD_REF; then you 
> want a virtual use from a BIT_FIELD_REF
>
This doesn't make much sense.  You should always get a virtual use
because the variable was a structure to begin with.  So, it doesn't
matter if fold() changes a COMPONENT_REF into a BIT_FIELD_REF.

>  -- indeed that's why my testcase 
> failed *with current unpatched treessa*.  Take a look at what the test case 
> does.
> 
Huh?  Current tree-ssa handles this test case just fine.

$ gcc-ssa --version
gcc-ssa (GCC) 3.5-tree-ssa 20040323 (Fedora Core Rawhide 3.5ssa-snapshot)
[ ... ]
$ gcc-ssa -O2 a.c -o a
$ ./a
$ echo $?
0

> If the marked VUSE is not there, dom somehow believes that i_2 is ok as well 
> and threads through the jump to L3, because the same test was false above.  
>
Right.  But the branch already gets this test case right.  Are you sure
you don't have other changes in your tree?

The theory of operation in the statement scanner is that whether a
variable needs a virtual or a real operand should be an intrinsic
property of the variable, not its context.  So, if we are inconsistently
choosing real and virtual operands for a variable that is accessed with
BIT_FIELD_REF, something else is wrong.

That is why get_virtual_var() should not care if the variable is a
gimple register or not.  We only handle context in
get_{stmt,expr}_operands and, to a limited extent, in add_stmt_operand. 
Since BIT_FIELD_REF may contain both virtual and real operands, we
should strip it before calling add_stmt_operand.

Otherwise, with your change to get_virtual_var(), if a variable V is a
gimple reg, get_virtual_var (BIT_FIELD_REF <V, 0, 32>) will return the
unmodified BIT_FIELD_REF and we will end up not adding an operand for V.

The reason why you needed to add '&& !is_gimple_reg (TREE_CODE ...)' to
the get_virtual_var() hunk is because the determination of whether the
operand was real or virtual was incorrectly done before the call to
get_virtual_var() in add_stmt_operand():

  /* If the original variable is not a scalar, it will be added to the list
     of virtual operands.  In that case, use its base symbol as the virtual
     variable representing it.  */
  is_real_op = is_gimple_reg (var);
  if (!is_real_op && !DECL_P (var))
    var = get_virtual_var (var);

Notice that since VAR is BIT_FIELD_REF <V, 0, 32>, before calling
get_virtual_var we have already determined that the operand is going to
be virtual (is_real_op is false).  In the case of 'V' this is wrong. 
That's why you were getting the verifier ICE.  And, indeed, that is why
I'm suggesting that we never handle BIT_FIELD_REF in here.

Instead, we should strip BIT_FIELD_REF before calling add_stmt_operand. 
Which means that we should not handle BIT_FIELD_REF together with
ARRAY_REF/COMPONENT_REF.

Since we are tightening things here, we should add something like this
to add_stmt_operand()

#if defined ENABLE_CHECKING
  if (!SSA_VAR_P (var)
      && TREE_CODE (var) != ARRAY_REF
      && TREE_CODE (var) != COMPONENT_REF
      && TREE_CODE (var) != REALPART_EXPR
      && TREE_CODE (var) != IMAGPART_EXPR
      && TREE_CODE (var) != ADDR_EXPR)
    abort ();
#endif

Those are all the things that add_stmt_operand() knows how to handle.  I
*think* this should never trigger.  If it does, we have more latent
bugs.


Thanks.  Diego.


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