This is the mail archive of the gcc@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: paradoxical subreg problem [ patch attached ]


 In message <200201282147.NAA19688@rtl.cygnus.com>, Jim Wilson writes:
 > 1) The extra bits are don't care bits.  This usage can occur anywhere, and
 >    is primarily used when manipulating values larger than the word size.
 >    This perhaps requires that the operand be a register, but I'm not sure.
 >    For instance, on a 32-bit target, given (subreg:DI (reg:SI 100)), the
 >    extra 32-bits are don't care bits.
OK.  This is basically I've always understood a paradoxical subreg to mean;
I don't recall a requirement that this only apply to registers, but that
restriction makes a fair amount of sense.


 > 2) The extra bits are known to be zero or one, depending on whether loads
 >    zero or sign extend by default.  This is only used for values smaller 
than
 >    or equal to the the word size, and I think this also requires that the
 >    operand be a MEM, but I'm not positive about the last condition.  For
 >    instance, on a 32-bit target with zero-extending loads, given
 >    (subreg:SI (mem:QI ...)), the extra 24-bits are known to be zero.  This
 >    usage occurs only between combine and reload.
This one is new to me, but appears to correspond to parts of combine that
I reviewed yesterday.   Based on my reading of combine, I suspect this 
requires a MEM.  And as pointed out later, it also requires LOAD_EXTEND_OP --
else the bits are undefined.

[ ... ]
Thanks.  This was extremely helpful in working through this issue.




So let's pretend we're in try_combine with the following:

[ Note the current sources will not exhibit this problem due to unrelated
  changes in how we order arguments for a comparison.  However, the
  underlying bug is very real. ]

(gdb) p debug_rtx(i3)
(jump_insn 83 82 92 (set (pc)
        (if_then_else (eq (reg:SI 107)
                (reg/v:SI 94))
            (label_ref 45)
            (pc))) 44 {absdi2+5} (insn_list 82 (nil))
    (expr_list:REG_DEAD (reg:SI 107)
        (expr_list:REG_BR_PROB (const_int 3999 [0xf9f])
            (nil))))

(gdb) p debug_rtx(i2)
(insn 82 79 83 (set (reg:SI 107)
        (zero_extend:SI (subreg:QI (reg/v:SI 94) 0))) 135 {zero_extendqidi2-1} 
(insn_list 11 (nil))
    (nil))

(gdb) p debug_rtx(i1)
(insn 11 7 14 (set (reg/v:SI 94)
        (mem/s:SI (plus:SI (reg:SI 3 %r3)
                (const_int 12 [0xc])) 1)) 69 {pre_ldw-4} (nil)
    (nil))


The first thing we do is substitute the SET_SRC of i2 into i3 giving us

(jump_insn 83 82 92 (set (pc)
        (if_then_else (eq (zero_extend:SI (subreg:QI (reg/v:SI 94) 0))
                (reg/v:SI 94))
            (label_ref 45)
            (pc))) 44 {absdi2+5} (insn_list 82 (nil))
    (expr_list:REG_DEAD (reg:SI 107)
        (expr_list:REG_BR_PROB (const_int 3999 [0xf9f])
            (nil))))


i1 is unchanged at this point, and i2 would be deleted if the combination
is a success.  I think we would all agree this transformation has not changed
the meaning of the code.

The next step is to substitute the SET_SRC of i1 into i3.  This happens in
several steps as (reg 94) appears twice in i3.

The first step creates an insn which looks something like this:

(jump_insn 83 82 92 (set (pc)
        (if_then_else (eq (zero_extend:SI (mem/s:QI (plus:SI (reg:SI 3 %r3)
                            (const_int 15 [0xf])) 1))
                (reg/v:SI 94))
            (label_ref 45)
            (pc))) 44 {absdi2+5} (insn_list 82 (nil))
    (expr_list:REG_DEAD (reg:SI 107)
        (expr_list:REG_BR_PROB (const_int 3999 [0xf9f])
            (nil))))


We then simplify the zero_extension into a subreg, which is OK given #2 in the
semantics Jim detailed.  The result of that simplification looks like:

(jump_insn 83 82 92 (set (pc)
        (if_then_else (eq (subreg:SI (mem/s:QI (plus:SI (reg:SI 3 %r3)
                            (const_int 15 [0xf])) 1) 0)
                (reg/v:SI 94))
            (label_ref 45)
            (pc))) 44 {absdi2+5} (insn_list 82 (nil))
    (expr_list:REG_DEAD (reg:SI 107)
        (expr_list:REG_BR_PROB (const_int 3999 [0xf9f])
            (nil))))


Now we proceed to replace the other occurrence of (reg 94) with its
memory location.  That results in the following insn:

(jump_insn 83 82 92 (set (pc)
        (if_then_else (eq (subreg:SI (mem/s:QI (plus:SI (reg:SI 3 %r3)
                            (const_int 15 [0xf])) 1) 0)
                (mem/s:SI (plus:SI (reg:SI 3 %r3)
                        (const_int 12 [0xc])) 1))
            (label_ref 45)
            (pc))) 44 {absdi2+5} (insn_list 82 (nil))
    (expr_list:REG_DEAD (reg:SI 107)
        (expr_list:REG_BR_PROB (const_int 3999 [0xf9f])
            (nil))))

Again, given Jim's semantics, we're still OK at this point.  We proceed to
call combine_simplify_rtx on the (eq ...) expression.  We simplify it to


(eq (subreg:SI (mem/s:QI (plus:SI (reg:SI 3 %r3)
                (const_int 15 [0xf])) 1) 0)
    (subreg:SI (mem/s:QI (plus:SI (reg:SI 3 %r3)
                (const_int 15 [0xf])) 1) 0))

Which is wrong as the second operand of the EQ has been incorrectly changed.


The culprit is this code in combine.c::simplify_comparison:

[ Kenner, does this look familiar :-) ]

  /* Now make any compound operations involved in this comparison.  Then,
     check for an outmost SUBREG on OP0 that is not doing anything or is
     paradoxical.  The latter case can only occur when it is known that the
     "extra" bits will be zero.  Therefore, it is safe to remove the SUBREG.
     We can never remove a SUBREG for a non-equality comparison because the
     sign bit is in a different place in the underlying object.  */

  op0 = make_compound_operation (op0, op1 == const0_rtx ? COMPARE : SET);
  op1 = make_compound_operation (op1, SET);

  if (GET_CODE (op0) == SUBREG && subreg_lowpart_p (op0)
      && GET_MODE_CLASS (GET_MODE (op0)) == MODE_INT
      && (code == NE || code == EQ)
      && ((GET_MODE_SIZE (GET_MODE (op0))
           > GET_MODE_SIZE (GET_MODE (SUBREG_REG (op0))))))
    {
      op0 = SUBREG_REG (op0);
      op1 = gen_lowpart_for_combine (GET_MODE (op0), op1);
    }

The compound operations look OK.

(gdb) p debug_rtx(op0)
(subreg:SI (mem/s:QI (plus:SI (reg:SI 3 %r3)
            (const_int 15 [0xf])) 1) 0)

(gdb) p debug_rtx(op1)
(mem/s:SI (plus:SI (reg:SI 3 %r3)
        (const_int 12 [0xc])) 1)


However, the logic for removing the SUBREG seems wrong.

Consider that we're comparing op0 and op1 for equality -- both operands
are SImode, so we're doing a 32bit comparison.  Narrowing the operands
to a smaller mode effectively removes bits from consideration.  This 
transformation is valid if and only if we know those bits have identical
values for *both* operands.

[ Time to go band and re-review the semantics. ]

So it seems like there's three cases to consider.

  1. SUBREG_REG (op0)) is a register.  In this case the bits are don't
  care bits and we can assume they have any convenient value.  So
  making the transformation is safe.

  2. SUBREG_REG (op0) is a memory and LOAD_EXTEND_OP is not defined.  In
  this case the upper bits of op0 are undefined.  It seems to me we should
  not make the simplification in that case as we do not know the contents
  of those bits.

  3. SUBREG_REG (op0) is a memory and LOAD_EXTEND_OP is defined.  In that
  case we know those bits are zeros or ones; however we do not know if
  they are the same as the upper bits of op1.  It seems to me we should
  not make the simplification in this case.


The net result is this simplification should only be valid if
SUBREG_REG (op0) is a register.  

With that in mind, this patch fixes the problem.  I'm testing this in an
older tree which exhibits the bug -- I'll refrain from checking this
in until folks have had a chance to comment.


	* combine.c (simplify_comparison): Avoid narrowing a comparison
	with a paradoxical subreg when doing so would drop signficant bits.

Index: combine.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gcc/combine.c,v
retrieving revision 1.227.6.4
diff -c -3 -p -r1.227.6.4 combine.c
*** combine.c	2001/02/13 05:06:59	1.227.6.4
--- combine.c	2002/01/29 16:18:53
*************** simplify_comparison (code, pop0, pop1)
*** 10912,10917 ****
--- 10912,10918 ----
    op1 = make_compound_operation (op1, SET);
  
    if (GET_CODE (op0) == SUBREG && subreg_lowpart_p (op0)
+       && GET_CODE (SUBREG_REG (op0)) == REG
        && GET_MODE_CLASS (GET_MODE (op0)) == MODE_INT
        && (code == NE || code == EQ)
        && ((GET_MODE_SIZE (GET_MODE (op0))



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