diff mbox series

[AArch64] Restrict POST_INC operand in aarch64_simd_mem_operand_p to register

Message ID 8D47A986-1D89-4AD6-8DD7-90563EA74C99@theobroma-systems.com
State New
Headers show
Series [AArch64] Restrict POST_INC operand in aarch64_simd_mem_operand_p to register | expand

Commit Message

Dominik Inführ Oct. 31, 2017, 1:47 p.m. UTC 
Hi,

I have a custom optimization pass, that moves an expression into an POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check POST_INC’s operand. Here is a patch that fixes this for me, although I am not sure if this is the right way to address this. GCC bootstraps and it causes no test regressions.

Dominik

ChangeLog:
2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>

	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
	if register given as operand for POST_INC.

-----

Comments

Kyrill Tkachov Oct. 31, 2017, 2:10 p.m. UTC | #1 
[cc'ing aarch64 maintainers]

Hi Dominik,

On 31/10/17 13:47, Dominik Inführ wrote:
> Hi,
>
> I have a custom optimization pass, that moves an expression into an POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check POST_INC’s operand. Here is a patch that fixes this for me, although I am not sure if this is the right way to address this. GCC bootstraps and it causes no test regressions.

The documentation for POST_INC says that its operand has to be a MEM or 
a REG.
Anything else is invalid RTL.
AArch64 only supports REGs as an operand to POST_INC.
The existing implementation of aarch64_simd_lane_bounds indeed doesn't 
properly reject non-REG operands to
POST_INC, so I think your patch is correct, in that we should be tighter 
to make sure we don't accept any MEMs.
But you'll need an approval from an aarch64 maintainer before committing.

However, I suggest you check your pass to make sure it doesn't try to 
put any arbitrary RTL into a POST_INC, because
otherwise some other parts of the compiler might blow up.

Thanks,
Kyrill

> Dominik
>
> ChangeLog:
> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
>
> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
> 	if register given as operand for POST_INC.
>
> -----
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index ed30b8c..bb61506 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
>   bool
>   aarch64_simd_mem_operand_p (rtx op)
>   {
> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
> -                       || REG_P (XEXP (op, 0)));
> +  if (!MEM_P (op))
> +    return false;
> +
> +  rtx opnd = XEXP (op, 0);
> +
> +  if (GET_CODE (opnd) == POST_INC)
> +    opnd = XEXP(opnd, 0);
> +
> +  return REG_P (opnd);
>   }
>
>   /* Emit a register copy from operand to operand, taking care not to
Dominik Inführ Oct. 31, 2017, 2:44 p.m. UTC | #2 
> On 31 Oct 2017, at 15:10, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
> [cc'ing aarch64 maintainers]
> 
> Hi Dominik,
> 
> On 31/10/17 13:47, Dominik Inführ wrote:
>> Hi,
>> 
>> I have a custom optimization pass, that moves an expression into an POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check POST_INC’s operand. Here is a patch that fixes this for me, although I am not sure if this is the right way to address this. GCC bootstraps and it causes no test regressions.
> 
> The documentation for POST_INC says that its operand has to be a MEM or a REG.
> Anything else is invalid RTL.
> AArch64 only supports REGs as an operand to POST_INC.
> The existing implementation of aarch64_simd_lane_bounds indeed doesn't properly reject non-REG operands to
> POST_INC, so I think your patch is correct, in that we should be tighter to make sure we don't accept any MEMs.
> But you'll need an approval from an aarch64 maintainer before committing.

Thanks!

> 
> However, I suggest you check your pass to make sure it doesn't try to put any arbitrary RTL into a POST_INC, because
> otherwise some other parts of the compiler might blow up.

Even if the pass then uses recog() to determine if this is still some valid instruction? Right now recog() succeeds with some arbitrary expression for POST_INC.

> 
> Thanks,
> Kyrill
> 
>> Dominik
>> 
>> ChangeLog:
>> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
>> 
>> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
>> 	if register given as operand for POST_INC.
>> 
>> -----
>> 
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index ed30b8c..bb61506 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
>>  bool
>>  aarch64_simd_mem_operand_p (rtx op)
>>  {
>> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
>> -                       || REG_P (XEXP (op, 0)));
>> +  if (!MEM_P (op))
>> +    return false;
>> +
>> +  rtx opnd = XEXP (op, 0);
>> +
>> +  if (GET_CODE (opnd) == POST_INC)
>> +    opnd = XEXP(opnd, 0);
>> +
>> +  return REG_P (opnd);
>>  }
>> 
>>  /* Emit a register copy from operand to operand, taking care not to
>
Kyrill Tkachov Oct. 31, 2017, 2:47 p.m. UTC | #3 
On 31/10/17 14:44, Dominik Inführ wrote:
>> On 31 Oct 2017, at 15:10, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>>
>> [cc'ing aarch64 maintainers]
>>
>> Hi Dominik,
>>
>> On 31/10/17 13:47, Dominik Inführ wrote:
>>> Hi,
>>>
>>> I have a custom optimization pass, that moves an expression into an POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check POST_INC’s operand. Here is a patch that fixes this for me, although I am not sure if this is the right way to address this. GCC bootstraps and it causes no test regressions.
>> The documentation for POST_INC says that its operand has to be a MEM or a REG.
>> Anything else is invalid RTL.
>> AArch64 only supports REGs as an operand to POST_INC.
>> The existing implementation of aarch64_simd_lane_bounds indeed doesn't properly reject non-REG operands to
>> POST_INC, so I think your patch is correct, in that we should be tighter to make sure we don't accept any MEMs.
>> But you'll need an approval from an aarch64 maintainer before committing.
> Thanks!
>
>> However, I suggest you check your pass to make sure it doesn't try to put any arbitrary RTL into a POST_INC, because
>> otherwise some other parts of the compiler might blow up.
> Even if the pass then uses recog() to determine if this is still some valid instruction? Right now recog() succeeds with some arbitrary expression for POST_INC.

I think that's still dangerous. AFAIK RTL functions, including recog (),
are not obliged to handle invalid RTL gracefully (they can assume it 
doesn't get passed),
so there is at least a theoretical risk of something blowing up in the 
future.

Kyrill

>
>> Thanks,
>> Kyrill
>>
>>> Dominik
>>>
>>> ChangeLog:
>>> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
>>>
>>> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
>>> 	if register given as operand for POST_INC.
>>>
>>> -----
>>>
>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>> index ed30b8c..bb61506 100644
>>> --- a/gcc/config/aarch64/aarch64.c
>>> +++ b/gcc/config/aarch64/aarch64.c
>>> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
>>>   bool
>>>   aarch64_simd_mem_operand_p (rtx op)
>>>   {
>>> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
>>> -                       || REG_P (XEXP (op, 0)));
>>> +  if (!MEM_P (op))
>>> +    return false;
>>> +
>>> +  rtx opnd = XEXP (op, 0);
>>> +
>>> +  if (GET_CODE (opnd) == POST_INC)
>>> +    opnd = XEXP(opnd, 0);
>>> +
>>> +  return REG_P (opnd);
>>>   }
>>>
>>>   /* Emit a register copy from operand to operand, taking care not to
Dominik Inführ Oct. 31, 2017, 2:53 p.m. UTC | #4 
> On 31 Oct 2017, at 15:47, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
> 
> On 31/10/17 14:44, Dominik Inführ wrote:
>>> On 31 Oct 2017, at 15:10, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>>> 
>>> [cc'ing aarch64 maintainers]
>>> 
>>> Hi Dominik,
>>> 
>>> On 31/10/17 13:47, Dominik Inführ wrote:
>>>> Hi,
>>>> 
>>>> I have a custom optimization pass, that moves an expression into an POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check POST_INC’s operand. Here is a patch that fixes this for me, although I am not sure if this is the right way to address this. GCC bootstraps and it causes no test regressions.
>>> The documentation for POST_INC says that its operand has to be a MEM or a REG.
>>> Anything else is invalid RTL.
>>> AArch64 only supports REGs as an operand to POST_INC.
>>> The existing implementation of aarch64_simd_lane_bounds indeed doesn't properly reject non-REG operands to
>>> POST_INC, so I think your patch is correct, in that we should be tighter to make sure we don't accept any MEMs.
>>> But you'll need an approval from an aarch64 maintainer before committing.
>> Thanks!
>> 
>>> However, I suggest you check your pass to make sure it doesn't try to put any arbitrary RTL into a POST_INC, because
>>> otherwise some other parts of the compiler might blow up.
>> Even if the pass then uses recog() to determine if this is still some valid instruction? Right now recog() succeeds with some arbitrary expression for POST_INC.
> 
> I think that's still dangerous. AFAIK RTL functions, including recog (),
> are not obliged to handle invalid RTL gracefully (they can assume it doesn't get passed),
> so there is at least a theoretical risk of something blowing up in the future.

Oh, thanks that’s good to know!

Dominik

> 
> Kyrill
> 
>> 
>>> Thanks,
>>> Kyrill
>>> 
>>>> Dominik
>>>> 
>>>> ChangeLog:
>>>> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
>>>> 
>>>> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
>>>> 	if register given as operand for POST_INC.
>>>> 
>>>> -----
>>>> 
>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>>> index ed30b8c..bb61506 100644
>>>> --- a/gcc/config/aarch64/aarch64.c
>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
>>>>  bool
>>>>  aarch64_simd_mem_operand_p (rtx op)
>>>>  {
>>>> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
>>>> -                       || REG_P (XEXP (op, 0)));
>>>> +  if (!MEM_P (op))
>>>> +    return false;
>>>> +
>>>> +  rtx opnd = XEXP (op, 0);
>>>> +
>>>> +  if (GET_CODE (opnd) == POST_INC)
>>>> +    opnd = XEXP(opnd, 0);
>>>> +
>>>> +  return REG_P (opnd);
>>>>  }
>>>> 
>>>>  /* Emit a register copy from operand to operand, taking care not to
Dominik Inführ Nov. 10, 2017, 3:43 p.m. UTC | #5 
Ping

> On 31 Oct 2017, at 14:47, Dominik Inführ <dominik.infuehr@theobroma-systems.com> wrote:
> 
> Hi,
> 
> I have a custom optimization pass, that moves an expression into an POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check POST_INC’s operand. Here is a patch that fixes this for me, although I am not sure if this is the right way to address this. GCC bootstraps and it causes no test regressions.
> 
> Dominik
> 
> ChangeLog:
> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
> 
> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
> 	if register given as operand for POST_INC.
> 
> -----
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index ed30b8c..bb61506 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
> bool
> aarch64_simd_mem_operand_p (rtx op)
> {
> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
> -                       || REG_P (XEXP (op, 0)));
> +  if (!MEM_P (op))
> +    return false;
> +
> +  rtx opnd = XEXP (op, 0);
> +
> +  if (GET_CODE (opnd) == POST_INC)
> +    opnd = XEXP(opnd, 0);
> +
> +  return REG_P (opnd);
> }
> 
> /* Emit a register copy from operand to operand, taking care not to
James Greenhalgh Nov. 17, 2017, 10:18 p.m. UTC | #6 
On Tue, Oct 31, 2017 at 02:47:29PM +0100, Dominik Inführ wrote:
> Hi,
> 
> I have a custom optimization pass, that moves an expression into an
> POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be
> true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check
> POST_INC’s operand. Here is a patch that fixes this for me, although I am not
> sure if this is the right way to address this. GCC bootstraps and it causes
> no test regressions.

OK!

Reviewed-by: James Greenhalgh <james.greenhalgh@arm.com>

Thanks,
James

> 
> Dominik
> 
> ChangeLog:
> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
> 
> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
> 	if register given as operand for POST_INC.
> 
> -----
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index ed30b8c..bb61506 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
>  bool
>  aarch64_simd_mem_operand_p (rtx op)
>  {
> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
> -                       || REG_P (XEXP (op, 0)));
> +  if (!MEM_P (op))
> +    return false;
> +
> +  rtx opnd = XEXP (op, 0);
> +
> +  if (GET_CODE (opnd) == POST_INC)
> +    opnd = XEXP(opnd, 0);
> +
> +  return REG_P (opnd);
>  }
> 
>  /* Emit a register copy from operand to operand, taking care not to
Dominik Inführ Nov. 20, 2017, 12:58 p.m. UTC | #7 
Thanks for the review! Could you also please commit this patch for me? I don’t have commit rights.

Thanks,
Dominik

> On 17 Nov 2017, at 23:18, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> 
> On Tue, Oct 31, 2017 at 02:47:29PM +0100, Dominik Inführ wrote:
>> Hi,
>> 
>> I have a custom optimization pass, that moves an expression into an
>> POST_INC-expression. GCC then ICE’s in df-scan.c since it expects REG_P to be
>> true for POST_INC’s operand. aarch64_simd_mem_operand_p doesn’t seem to check
>> POST_INC’s operand. Here is a patch that fixes this for me, although I am not
>> sure if this is the right way to address this. GCC bootstraps and it causes
>> no test regressions.
> 
> OK!
> 
> Reviewed-by: James Greenhalgh <james.greenhalgh@arm.com>
> 
> Thanks,
> James
> 
>> 
>> Dominik
>> 
>> ChangeLog:
>> 2017-10-31  Dominik Infuehr  <dominik.infuehr@theobroma-systems.com>
>> 
>> 	* config/aarch64/aarch64.c (aarch64_simd_mem_operand_p): Check
>> 	if register given as operand for POST_INC.
>> 
>> -----
>> 
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index ed30b8c..bb61506 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -11850,8 +11850,15 @@ aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
>> bool
>> aarch64_simd_mem_operand_p (rtx op)
>> {
>> -  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
>> -                       || REG_P (XEXP (op, 0)));
>> +  if (!MEM_P (op))
>> +    return false;
>> +
>> +  rtx opnd = XEXP (op, 0);
>> +
>> +  if (GET_CODE (opnd) == POST_INC)
>> +    opnd = XEXP(opnd, 0);
>> +
>> +  return REG_P (opnd);
>> }
>> 
>> /* Emit a register copy from operand to operand, taking care not to
> 
>
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index ed30b8c..bb61506 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11850,8 +11850,15 @@  aarch64_simd_lane_bounds (rtx operand, HOST_WIDE_INT low, HOST_WIDE_INT high,
 bool
 aarch64_simd_mem_operand_p (rtx op)
 {
-  return MEM_P (op) && (GET_CODE (XEXP (op, 0)) == POST_INC
-                       || REG_P (XEXP (op, 0)));
+  if (!MEM_P (op))
+    return false;
+
+  rtx opnd = XEXP (op, 0);
+
+  if (GET_CODE (opnd) == POST_INC)
+    opnd = XEXP(opnd, 0);
+
+  return REG_P (opnd);
 }

 /* Emit a register copy from operand to operand, taking care not to