diff mbox

[PR64971] Convert function pointer to Pmode when emit call

Message ID 57A35211.6090405@foss.arm.com
State New
Headers show

Commit Message

Renlin Li Aug. 4, 2016, 2:32 p.m. UTC
Hi all,

In the case of PR64971 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64971),
the compiler ICE when compiling gcc.c-torture/compile/pr37433.c with 
ilp32 abi.

As we know, in aarch64 ilp32, the ptr_mode is SImode while Pmode is 
still DImode. It means all address should be DImode, and the backend 
defines the patterns with this assumption.

The generic part expand_expr_addr_expr () function however generates a
SYMBOL_REF with SImode, it's later used as the address of a MEM rtx 
pattern in a call expression. There is no matching pattern for this 
SImode address, that's why gcc ICEs.
(symbol_ref/f:SI ("*.LC0") [flags 0x82] <var_decl 0x3ffb7fa1200 *.LC0>)

But here, I think what expand_expr_addr_expr does is correct. In this
particular case, expand_expr_addr_expr is not generating an address. 
According to the source code, it's generating a function pointer, and 
later this pointer is used in a call expression. So SImode should be 
right in this case.

The behavior of the test case is, get the address of a piece of memory, 
cast it into a function pointer, and call the function. IIUC, the flow 
is like this:
CALL_EXPR ( NOP_EXPR (ADDR_EXPR ()))

NOP_EXPR here is to convert the address into a function pointer which
should be ptr_mode (SImode). So it's the responsibility of call expander
to convert the pointer into Pmode to create legal call rtx patern.

In the test case, there are two functions. The first function generates 
function calls with a SYMBOL_REF as address, the second one generates a 
REG as address. They are all of ptr_mode.
However, prepare_call_address () will convert the REG into Pmode to make 
it as a legal address while SYMBOL_REF is missed. That's why I add the 
code there.

And I want to change the PR64971 into a middle-end issue. The ICE 
manifests in aarch64 target, but I believe this should be a generic 
problem for targets which define ptr_mode different from Pmode.

There is a test case already, so I didn't add one.
aarch64-none-elf regression test Okay, aarch64-linux bootstrap Okay.
But I believe this may not help as the default abi is LP64.

It will be great if Andrew you can help to do regression test in your
aarch64 ilp32 environment.

And I double checked that, the backend fix can be removed without any
problem. It's good to expose middle-end bugs.

Okay for trunk and backport to branch 6?

gcc/ChangeLog:

2016-08-04  Renlin Li  <renlin.li@arm.com>

         PR middle-end/64971
	* calls.c (prepare_call_address): Convert funexp to Pmode when
         necessary.
	* config/aarch64/aarch64.md (sibcall): Remove fix for PR 64971.
	(sibcall_value): Likewise.

Comments

Jeff Law Aug. 5, 2016, 8:22 p.m. UTC | #1
On 08/04/2016 08:32 AM, Renlin Li wrote:
> Hi all,
>
> In the case of PR64971
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64971),
> the compiler ICE when compiling gcc.c-torture/compile/pr37433.c with
> ilp32 abi.
>
> As we know, in aarch64 ilp32, the ptr_mode is SImode while Pmode is
> still DImode. It means all address should be DImode, and the backend
> defines the patterns with this assumption.
>
> The generic part expand_expr_addr_expr () function however generates a
> SYMBOL_REF with SImode, it's later used as the address of a MEM rtx
> pattern in a call expression. There is no matching pattern for this
> SImode address, that's why gcc ICEs.
> (symbol_ref/f:SI ("*.LC0") [flags 0x82] <var_decl 0x3ffb7fa1200 *.LC0>)
>
> But here, I think what expand_expr_addr_expr does is correct. In this
> particular case, expand_expr_addr_expr is not generating an address.
> According to the source code, it's generating a function pointer, and
> later this pointer is used in a call expression. So SImode should be
> right in this case.
>
> The behavior of the test case is, get the address of a piece of memory,
> cast it into a function pointer, and call the function. IIUC, the flow
> is like this:
> CALL_EXPR ( NOP_EXPR (ADDR_EXPR ()))
>
> NOP_EXPR here is to convert the address into a function pointer which
> should be ptr_mode (SImode). So it's the responsibility of call expander
> to convert the pointer into Pmode to create legal call rtx patern.
>
> In the test case, there are two functions. The first function generates
> function calls with a SYMBOL_REF as address, the second one generates a
> REG as address. They are all of ptr_mode.
> However, prepare_call_address () will convert the REG into Pmode to make
> it as a legal address while SYMBOL_REF is missed. That's why I add the
> code there.
>
> And I want to change the PR64971 into a middle-end issue. The ICE
> manifests in aarch64 target, but I believe this should be a generic
> problem for targets which define ptr_mode different from Pmode.
>
> There is a test case already, so I didn't add one.
> aarch64-none-elf regression test Okay, aarch64-linux bootstrap Okay.
> But I believe this may not help as the default abi is LP64.
>
> It will be great if Andrew you can help to do regression test in your
> aarch64 ilp32 environment.
>
> And I double checked that, the backend fix can be removed without any
> problem. It's good to expose middle-end bugs.
>
> Okay for trunk and backport to branch 6?
>
> gcc/ChangeLog:
>
> 2016-08-04  Renlin Li  <renlin.li@arm.com>
>
>         PR middle-end/64971
>     * calls.c (prepare_call_address): Convert funexp to Pmode when
>         necessary.
>     * config/aarch64/aarch64.md (sibcall): Remove fix for PR 64971.
>     (sibcall_value): Likewise.
OK.

jeff
diff mbox

Patch

diff --git a/gcc/calls.c b/gcc/calls.c
index c04d00f..b00c153 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -194,10 +194,19 @@  prepare_call_address (tree fndecl_or_type, rtx funexp, rtx static_chain_value,
 	       && targetm.small_register_classes_for_mode_p (FUNCTION_MODE))
 	      ? force_not_mem (memory_address (FUNCTION_MODE, funexp))
 	      : memory_address (FUNCTION_MODE, funexp));
-  else if (! sibcallp)
+  else
     {
-      if (!NO_FUNCTION_CSE && optimize && ! flag_no_function_cse)
-	funexp = force_reg (Pmode, funexp);
+      /* funexp could be a SYMBOL_REF represents a function pointer which is
+	 of ptr_mode.  In this case, it should be converted into address mode
+	 to be a valid address for memory rtx pattern.  See PR 64971.  */
+      if (GET_MODE (funexp) != Pmode)
+	funexp = convert_memory_address (Pmode, funexp);
+
+      if (! sibcallp)
+	{
+	  if (!NO_FUNCTION_CSE && optimize && ! flag_no_function_cse)
+	    funexp = force_reg (Pmode, funexp);
+	}
     }
 
   if (static_chain_value != 0
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index f15dd8d..c95258b 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -859,13 +859,6 @@ 
 	   || aarch64_is_noplt_call_p (callee)))
       XEXP (operands[0], 0) = force_reg (Pmode, callee);
 
-    /* FIXME: This is a band-aid.  Need to analyze why expand_expr_addr_expr
-       is generating an SImode symbol reference.  See PR 64971.  */
-    if (TARGET_ILP32
-	&& GET_CODE (XEXP (operands[0], 0)) == SYMBOL_REF
-	&& GET_MODE (XEXP (operands[0], 0)) == SImode)
-      XEXP (operands[0], 0) = convert_memory_address (Pmode,
-						      XEXP (operands[0], 0));
     if (operands[2] == NULL_RTX)
       operands[2] = const0_rtx;
 
@@ -897,14 +890,6 @@ 
 	   || aarch64_is_noplt_call_p (callee)))
       XEXP (operands[1], 0) = force_reg (Pmode, callee);
 
-    /* FIXME: This is a band-aid.  Need to analyze why expand_expr_addr_expr
-       is generating an SImode symbol reference.  See PR 64971.  */
-    if (TARGET_ILP32
-	&& GET_CODE (XEXP (operands[1], 0)) == SYMBOL_REF
-	&& GET_MODE (XEXP (operands[1], 0)) == SImode)
-      XEXP (operands[1], 0) = convert_memory_address (Pmode,
-						      XEXP (operands[1], 0));
-
     if (operands[3] == NULL_RTX)
       operands[3] = const0_rtx;