diff mbox

PATCH: PR rtl-optimization/54157: [x32] -maddress-mode=long failures

Message ID CAMe9rOr-P76Z7mFHjiLPuo4szyjny39mweiQcW-Wv4FTvu3VUA@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Aug. 7, 2012, 7:28 p.m. UTC 
On Sun, Aug 5, 2012 at 12:47 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> For the record, I can't approve this, but...
>
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>> i386,md has
>>
>> (define_expand "extzv"
>>   [(set (match_operand:SI 0 "register_operand")
>>         (zero_extract:SI (match_operand 1 "ext_register_operand")
>>                          (match_operand:SI 2 "const8_operand")
>>                          (match_operand:SI 3 "const8_operand")))]
>>   ""
>>
>> and mode_for_extraction picks word_mode for operand 1 since
>> its mode is VOIDmode.  This patch changes mode_for_extraction
>> to return the mode of operand 1 if the pattern accepts any mode.
>> I added *jcc_btsi_mask_2 since combine now tries a different
>> pattern, which leads to test failures on gcc.target/i386/bt-mask-1.c
>> and  gcc.target/i386/bt-mask-2.  I didn't update *jcc_btsi_mask_1
>> instead since I am not sure if it is used elsewhere.  Tested on
>> Linux/x86-64 and Linux/x32.  OK for trunk?
>
> the mode of the extraction operand is defined to be word_mode
> for registers (see md.texi), so that at least would need to
> be updated.  But I'm not convinced that the wanted_inner_mode here:
>
>    if (! in_dest && unsignedp
> -      && mode_for_extraction (EP_extzv, -1) != MAX_MACHINE_MODE)
> +      && mode_for_extraction (EP_extzv, -1, VOIDmode) != MAX_MACHINE_MODE)
>      {
> -      wanted_inner_reg_mode = mode_for_extraction (EP_extzv, 1);
> -      pos_mode = mode_for_extraction (EP_extzv, 3);
> -      extraction_mode = mode_for_extraction (EP_extzv, 0);
> +      wanted_inner_reg_mode = mode_for_extraction (EP_extzv, 1,
> +                                                  inner_mode);
> +      pos_mode = mode_for_extraction (EP_extzv, 3, VOIDmode);
> +      extraction_mode = mode_for_extraction (EP_extzv, 0, VOIDmode);
>      }
>
> is right.  inner_mode is the mode of the thing we're extracting,
> which doesn't ncessarily have anything to do with what the ext*
> patterns support.
>
> FWIW, in reply to your force_to_mode message, gen_lowpart_for_combine
> looks a bit odd:
>
>   if (omode == imode)
>     return x;
>
>   /* Return identity if this is a CONST or symbolic reference.  */
>   if (omode == Pmode
>       && (GET_CODE (x) == CONST
>           || GET_CODE (x) == SYMBOL_REF
>           || GET_CODE (x) == LABEL_REF))
>     return x;
>
> So if we know the modes are different, we nevertheless return the
> original mode for CONST, SYMBOL_REF or LABEL_REF.  Surely the caller
> isn't going to be expecting that?  If we're not prepared to change
> the mode to the one that the caller asked for then I think we should
> goto fail instead.
>
> I don't know if you're hitting that or not.
>
> Richard

Yes, I did

Breakpoint 5, gen_lowpart_for_combine (omode=DImode, x=0x7ffff1b03440)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:10778
10778	{
(const:SI (plus:SI (symbol_ref:SI ("tmp2") <var_decl 0x7ffff1997140 tmp2>)
        (const_int 99452 [0x1847c])))
(gdb) c
Continuing.

Breakpoint 5, gen_lowpart_for_combine (omode=DImode, x=0x7ffff1b03440)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:10778
10778	{
(const:SI (plus:SI (symbol_ref:SI ("tmp2") <var_decl 0x7ffff1997140 tmp2>)
        (const_int 99452 [0x1847c])))
(gdb) bt
#0  gen_lowpart_for_combine (omode=DImode, x=0x7ffff1b03440)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:10778
#1  0x0000000000c83805 in gen_lowpart_or_truncate (x=0x7ffff1b03440,
    mode=DImode) at /export/gnu/import/git/gcc-misc/gcc/combine.c:8110
#2  force_to_mode (x=0x7ffff1b02cd8, mode=DImode, mask=<optimized out>,
    just_select=<optimized out>)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:8389
#3  0x0000000000c8458a in make_extraction (mode=SImode, inner=0x7ffff1b02cd8,
    pos=2, pos_rtx=<optimized out>, len=2, unsignedp=1,
    in_dest=<optimized out>, in_compare=0)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:7480
#4  0x0000000000c86f9c in make_compound_operation (x=x@entry=0x7ffff1b02d68,
    in_code=in_code@entry=SET)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:7863
#5  0x0000000000c89924 in simplify_set (x=0x7ffff1af7c78)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:6539
#6  combine_simplify_rtx (x=0x7ffff1af7c78, op0_mode=VOIDmode,
    in_dest=in_dest@entry=0, in_cond=in_cond@entry=0)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:5971
#7  0x0000000000c8bd1b in subst (x=<optimized out>, from=<optimized out>,
    to=<optimized out>, in_dest=0, in_cond=0, unique_copy=0)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:5301
#8  0x0000000000c8b8ab in subst (x=0x7ffff1aea870, from=0x7ffff1afc880,
---Type <return> to continue, or q <return> to quit---
    to=0x7ffff1b02cd8, in_dest=0, in_cond=0, unique_copy=0)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:5165
#9  0x0000000000c8f875 in try_combine (i3=i3@entry=0x7ffff1afe5a0,
    i2=i2@entry=0x7ffff1afe558, i1=<optimized out>, i0=<optimized out>,
    i0@entry=0x0, new_direct_jump_p=new_direct_jump_p@entry=0x7fffffffdb64,
    last_combined_insn=last_combined_insn@entry=0x7ffff1afe5a0)
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:3260
#10 0x0000000000c94673 in combine_instructions (nregs=<optimized out>,
    f=<optimized out>) at /export/gnu/import/git/gcc-misc/gcc/combine.c:1265
#11 rest_of_handle_combine ()
    at /export/gnu/import/git/gcc-misc/gcc/combine.c:13948
#12 0x0000000000824755 in execute_one_pass (
    pass=pass@entry=0x135aae0 <pass_combine>)
    at /export/gnu/import/git/gcc-misc/gcc/passes.c:2158
#13 0x0000000000824b15 in execute_pass_list (pass=0x135aae0 <pass_combine>)
    at /export/gnu/import/git/gcc-misc/gcc/passes.c:2213
#14 0x0000000000824b27 in execute_pass_list (
    pass=0x1355d20 <pass_rest_of_compilation>)
    at /export/gnu/import/git/gcc-misc/gcc/passes.c:2214
#15 0x0000000000610fb8 in expand_function (node=0x7ffff1995750)
    at /export/gnu/import/git/gcc-misc/gcc/cgraphunit.c:1610
#16 0x0000000000612df4 in expand_all_functions ()
    at /export/gnu/import/git/gcc-misc/gcc/cgraphunit.c:1715
---Type <return> to continue, or q <return> to quit---
#17 compile () at /export/gnu/import/git/gcc-misc/gcc/cgraphunit.c:2013
#18 0x00000000006133b5 in finalize_compilation_unit ()
    at /export/gnu/import/git/gcc-misc/gcc/cgraphunit.c:2090
#19 0x0000000000506b90 in c_write_global_declarations ()
    at /export/gnu/import/git/gcc-misc/gcc/c/c-decl.c:10116
#20 0x00000000008c5325 in compile_file ()
    at /export/gnu/import/git/gcc-misc/gcc/toplev.c:560
#21 0x00000000008c6eb8 in do_compile ()
    at /export/gnu/import/git/gcc-misc/gcc/toplev.c:1863
#22 toplev_main (argc=17, argv=0x7fffffffdde8)
    at /export/gnu/import/git/gcc-misc/gcc/toplev.c:1939
#23 0x0000003c88221735 in __libc_start_main () from /lib64/libc.so.6
#24 0x00000000004e9f41 in _start ()

This patch:

      constant integer or has a mode the same size.  */

works for the testcase.

Comments

Richard Sandiford Aug. 8, 2012, 8:08 a.m. UTC | #1 
"H.J. Lu" <hjl.tools@gmail.com> writes:
> diff --git a/gcc/combine.c b/gcc/combine.c
> index a07c046..b9a3589 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -10784,12 +10784,30 @@ gen_lowpart_for_combine (enum machine_mode omode, rtx
> x)
>    if (omode == imode)
>      return x;
>
> -  /* Return identity if this is a CONST or symbolic reference.  */
> -  if (omode == Pmode
> -      && (GET_CODE (x) == CONST
> -	  || GET_CODE (x) == SYMBOL_REF
> -	  || GET_CODE (x) == LABEL_REF))
> -    return x;
> +  if (omode == Pmode)
> +    {
> +      /* Return identity if this is a symbolic reference.  */
> +      if (GET_CODE (x) == SYMBOL_REF || GET_CODE (x) == LABEL_REF)
> +	return x;
> +
> +      /* Return identity for CONST unless this is a PLUS of 2 constant
> +	 operands.  */
> +      if (GET_CODE (x) == CONST)
> +	{
> +	  rtx y = XEXP (x, 0);
> +	  if (GET_CODE (y) == PLUS
> +	      && ((CONST_INT_P (XEXP (y, 1))
> +		   && (GET_CODE (XEXP (y, 0)) == CONST
> +		       || GET_CODE (XEXP (y, 0)) == SYMBOL_REF
> +		       || GET_CODE (XEXP (y, 0)) == LABEL_REF))
> +		  || (CONST_INT_P (XEXP (y, 1))
> +		      && (GET_CODE (XEXP (y, 0)) == CONST
> +			  || GET_CODE (XEXP (y, 0)) == SYMBOL_REF
> +			  || GET_CODE (XEXP (y, 0)) == LABEL_REF))))
> +	    goto fail;
> +	  return x;
> +	}
> +    }
>
>    /* We can only support MODE being wider than a word if X is a
>       constant integer or has a mode the same size.  */
>
> works for the testcase.

My point was that the "return x" is always wrong.  Whenever we return x
here, we know we're returning something in a different mode from the one
that the caller wanted.  Returning a Pmode LABEL_REF might not trigger
that plus_constant assert, but it's still wrong.

It looks like this came from the mips-rewrite branch:

2003-03-13  Richard Sandiford  <rsandifo@redhat.com>

        * combine.c (gen_lowpart_for_combine): Treat the lowpart Pmode of
        a CONST as identity.  Check the return value of gen_lowpart_common.

so I can categorically confirm that the person who wrote it didn't
know what they were doing.  It also means that this case was motivated
by an experiment to make Pmode == DImode for n32, which we ended up
discarding because it produced worse code.

If this case really is important, we might consider using
convert_memory_address (Pmode, x) instead.  I'm not sure whether
that would be right for targets with address spaces though, because
we don't know which address space is associated with the address.
Hopefully someone who knows address spaces can comment.

If it is correct, then it should probably go in gen_lowpart_common
rather than gen_lowpart_for_combine.

But given how few people have hit this, it doesn't look like a
particularly important attempted optimisation.  I'll pre-approve
a patch that undoes my mistake and simply removes:

  /* Return identity if this is a CONST or symbolic reference.  */
  if (omode == Pmode
      && (GET_CODE (x) == CONST
	  || GET_CODE (x) == SYMBOL_REF
	  || GET_CODE (x) == LABEL_REF))
    return x;

Richard
diff mbox

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index a07c046..b9a3589 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -10784,12 +10784,30 @@  gen_lowpart_for_combine (enum machine_mode omode, rtx
x)
   if (omode == imode)
     return x;

-  /* Return identity if this is a CONST or symbolic reference.  */
-  if (omode == Pmode
-      && (GET_CODE (x) == CONST
-	  || GET_CODE (x) == SYMBOL_REF
-	  || GET_CODE (x) == LABEL_REF))
-    return x;
+  if (omode == Pmode)
+    {
+      /* Return identity if this is a symbolic reference.  */
+      if (GET_CODE (x) == SYMBOL_REF || GET_CODE (x) == LABEL_REF)
+	return x;
+
+      /* Return identity for CONST unless this is a PLUS of 2 constant
+	 operands.  */
+      if (GET_CODE (x) == CONST)
+	{
+	  rtx y = XEXP (x, 0);
+	  if (GET_CODE (y) == PLUS
+	      && ((CONST_INT_P (XEXP (y, 1))
+		   && (GET_CODE (XEXP (y, 0)) == CONST
+		       || GET_CODE (XEXP (y, 0)) == SYMBOL_REF
+		       || GET_CODE (XEXP (y, 0)) == LABEL_REF))
+		  || (CONST_INT_P (XEXP (y, 1))
+		      && (GET_CODE (XEXP (y, 0)) == CONST
+			  || GET_CODE (XEXP (y, 0)) == SYMBOL_REF
+			  || GET_CODE (XEXP (y, 0)) == LABEL_REF))))
+	    goto fail;
+	  return x;
+	}
+    }

   /* We can only support MODE being wider than a word if X is a