Re: Make ivopts handle calls to internal functions

[Christophe Lyon]

Hi,


On 13 January 2018 at 16:34, Jeff Law  wrote:
> On 01/09/2018 08:23 AM, Richard Sandiford wrote:
>> Richard Biener  writes:
>>> On Mon, Nov 20, 2017 at 12:31 PM, Bin.Cheng  wrote:
 On Fri, Nov 17, 2017 at 3:03 PM, Richard Sandiford
  wrote:
> ivopts previously treated pointer arguments to internal functions
> like IFN_MASK_LOAD and IFN_MASK_STORE as normal gimple values.
> This patch makes it treat them as addresses instead.  This makes
> a significant difference to the code quality for SVE loops,
> since we can then use loads and stores with scaled indices.
 Thanks for working on this.  This can be extended to other internal
 functions which eventually
 are expanded into memory references.  I believe (at least) both x86
 and AArch64 has such
 requirement.
>>>
>>> In addition to Bins comments I only have a single one (the rest of the
>>> middle-end
>>> changes look OK).  The alias type of MEM_REFs and TARGET_MEM_REFs
>>> in ADDR_EXPR context is meaningless so you don't need to jump through hoops
>>> to get at it or preserve it in any way, likewise for CLIQUE/BASE if it
>>> were present.
>>
>> Ah, OK.
>>
>>> Maybe you can simplify code with this.
>>
>> In the end it didn't really simplify the code, since internal-fn.c
>> uses the address to build a (TARGET_)MEM_REF, and the alias information
>> of that ref needs to be correct, since it gets carried across to the
>> MEM rtx.  But it does mean that the alias_ptr_type check in the previous:
>>
>>   if (TREE_CODE (mem) == TARGET_MEM_REF
>> && types_compatible_p (TREE_TYPE (mem), type)
>> && alias_ptr_type == TREE_TYPE (TMR_OFFSET (mem))
>> && integer_zerop (TMR_OFFSET (mem)))
>>   return mem;
>>
>> made no sense: we should simply replace the TMR_OFFSET if it has
>> the wrong type.
>>
>>> As you're introducing _MEM_REF as a valid construct (it weren't
>>> before) you'll run into missing / misguided foldings eventually.  So
>>> be prepared to fix up fallout.
>>
>> OK :-) I haven't hit any new places yet, but like you say, I'll be on
>> the lookout.
>>
>> Is the version below OK?  Tested on aarch64-linux-gnu, x86_64-linux-gnu
>> and powerpc64le-linux-gnu.
>>
>> Richard
>>
>>
>> 2018-01-09  Richard Sandiford  
>>   Alan Hayward  
>>   David Sherwood  
>>
>> gcc/
>>   * expr.c (expand_expr_addr_expr_1): Handle ADDR_EXPRs of
>>   TARGET_MEM_REFs.
>>   * gimple-expr.h (is_gimple_addressable: Likewise.
>>   * gimple-expr.c (is_gimple_address): Likewise.
>>   * internal-fn.c (expand_call_mem_ref): New function.
>>   (expand_mask_load_optab_fn): Use it.
>>   (expand_mask_store_optab_fn): Likewise.
> OK.
> jeff


I've reported that the updated tests fail on aarch64-none-elf -mabi=ilp32:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83848

Christophe

Re: Make ivopts handle calls to internal functions

[Jeff Law]

On 01/09/2018 08:23 AM, Richard Sandiford wrote:
> Richard Biener  writes:
>> On Mon, Nov 20, 2017 at 12:31 PM, Bin.Cheng  wrote:
>>> On Fri, Nov 17, 2017 at 3:03 PM, Richard Sandiford
>>>  wrote:
 ivopts previously treated pointer arguments to internal functions
 like IFN_MASK_LOAD and IFN_MASK_STORE as normal gimple values.
 This patch makes it treat them as addresses instead.  This makes
 a significant difference to the code quality for SVE loops,
 since we can then use loads and stores with scaled indices.
>>> Thanks for working on this.  This can be extended to other internal
>>> functions which eventually
>>> are expanded into memory references.  I believe (at least) both x86
>>> and AArch64 has such
>>> requirement.
>>
>> In addition to Bins comments I only have a single one (the rest of the
>> middle-end
>> changes look OK).  The alias type of MEM_REFs and TARGET_MEM_REFs
>> in ADDR_EXPR context is meaningless so you don't need to jump through hoops
>> to get at it or preserve it in any way, likewise for CLIQUE/BASE if it
>> were present.
> 
> Ah, OK.
> 
>> Maybe you can simplify code with this.
> 
> In the end it didn't really simplify the code, since internal-fn.c
> uses the address to build a (TARGET_)MEM_REF, and the alias information
> of that ref needs to be correct, since it gets carried across to the
> MEM rtx.  But it does mean that the alias_ptr_type check in the previous:
> 
>   if (TREE_CODE (mem) == TARGET_MEM_REF
> && types_compatible_p (TREE_TYPE (mem), type)
> && alias_ptr_type == TREE_TYPE (TMR_OFFSET (mem))
> && integer_zerop (TMR_OFFSET (mem)))
>   return mem;
> 
> made no sense: we should simply replace the TMR_OFFSET if it has
> the wrong type.
> 
>> As you're introducing _MEM_REF as a valid construct (it weren't
>> before) you'll run into missing / misguided foldings eventually.  So
>> be prepared to fix up fallout.
> 
> OK :-) I haven't hit any new places yet, but like you say, I'll be on
> the lookout.
> 
> Is the version below OK?  Tested on aarch64-linux-gnu, x86_64-linux-gnu
> and powerpc64le-linux-gnu.
> 
> Richard
> 
> 
> 2018-01-09  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * expr.c (expand_expr_addr_expr_1): Handle ADDR_EXPRs of
>   TARGET_MEM_REFs.
>   * gimple-expr.h (is_gimple_addressable: Likewise.
>   * gimple-expr.c (is_gimple_address): Likewise.
>   * internal-fn.c (expand_call_mem_ref): New function.
>   (expand_mask_load_optab_fn): Use it.
>   (expand_mask_store_optab_fn): Likewise.
OK.
jeff

Re: Make ivopts handle calls to internal functions

[Richard Sandiford]

Richard Biener  writes:
> On Mon, Nov 20, 2017 at 12:31 PM, Bin.Cheng  wrote:
>> On Fri, Nov 17, 2017 at 3:03 PM, Richard Sandiford
>>  wrote:
>>> ivopts previously treated pointer arguments to internal functions
>>> like IFN_MASK_LOAD and IFN_MASK_STORE as normal gimple values.
>>> This patch makes it treat them as addresses instead.  This makes
>>> a significant difference to the code quality for SVE loops,
>>> since we can then use loads and stores with scaled indices.
>> Thanks for working on this.  This can be extended to other internal
>> functions which eventually
>> are expanded into memory references.  I believe (at least) both x86
>> and AArch64 has such
>> requirement.
>
> In addition to Bins comments I only have a single one (the rest of the
> middle-end
> changes look OK).  The alias type of MEM_REFs and TARGET_MEM_REFs
> in ADDR_EXPR context is meaningless so you don't need to jump through hoops
> to get at it or preserve it in any way, likewise for CLIQUE/BASE if it
> were present.

Ah, OK.

> Maybe you can simplify code with this.

In the end it didn't really simplify the code, since internal-fn.c
uses the address to build a (TARGET_)MEM_REF, and the alias information
of that ref needs to be correct, since it gets carried across to the
MEM rtx.  But it does mean that the alias_ptr_type check in the previous:

  if (TREE_CODE (mem) == TARGET_MEM_REF
  && types_compatible_p (TREE_TYPE (mem), type)
  && alias_ptr_type == TREE_TYPE (TMR_OFFSET (mem))
  && integer_zerop (TMR_OFFSET (mem)))
return mem;

made no sense: we should simply replace the TMR_OFFSET if it has
the wrong type.

> As you're introducing _MEM_REF as a valid construct (it weren't
> before) you'll run into missing / misguided foldings eventually.  So
> be prepared to fix up fallout.

OK :-) I haven't hit any new places yet, but like you say, I'll be on
the lookout.

Is the version below OK?  Tested on aarch64-linux-gnu, x86_64-linux-gnu
and powerpc64le-linux-gnu.

Richard


2018-01-09  Richard Sandiford  
Alan Hayward  
David Sherwood  

gcc/
* expr.c (expand_expr_addr_expr_1): Handle ADDR_EXPRs of
TARGET_MEM_REFs.
* gimple-expr.h (is_gimple_addressable: Likewise.
* gimple-expr.c (is_gimple_address): Likewise.
* internal-fn.c (expand_call_mem_ref): New function.
(expand_mask_load_optab_fn): Use it.
(expand_mask_store_optab_fn): Likewise.

Index: gcc/expr.c
===
--- gcc/expr.c  2018-01-09 15:13:32.603106251 +
+++ gcc/expr.c  2018-01-09 15:13:32.784098242 +
@@ -7885,6 +7885,9 @@ expand_expr_addr_expr_1 (tree exp, rtx t
return expand_expr (tem, target, tmode, modifier);
   }
 
+case TARGET_MEM_REF:
+  return addr_for_mem_ref (exp, as, true);
+
 case CONST_DECL:
   /* Expand the initializer like constants above.  */
   result = XEXP (expand_expr_constant (DECL_INITIAL (exp),
Index: gcc/gimple-expr.h
===
--- gcc/gimple-expr.h   2018-01-09 15:13:32.603106251 +
+++ gcc/gimple-expr.h   2018-01-09 15:13:32.785098198 +
@@ -119,6 +119,7 @@ virtual_operand_p (tree op)
 is_gimple_addressable (tree t)
 {
   return (is_gimple_id (t) || handled_component_p (t)
+ || TREE_CODE (t) == TARGET_MEM_REF
  || TREE_CODE (t) == MEM_REF);
 }
 
Index: gcc/gimple-expr.c
===
--- gcc/gimple-expr.c   2018-01-09 15:13:32.603106251 +
+++ gcc/gimple-expr.c   2018-01-09 15:13:32.784098242 +
@@ -631,7 +631,9 @@ is_gimple_address (const_tree t)
   op = TREE_OPERAND (op, 0);
 }
 
-  if (CONSTANT_CLASS_P (op) || TREE_CODE (op) == MEM_REF)
+  if (CONSTANT_CLASS_P (op)
+  || TREE_CODE (op) == TARGET_MEM_REF
+  || TREE_CODE (op) == MEM_REF)
 return true;
 
   switch (TREE_CODE (op))
Index: gcc/internal-fn.c
===
--- gcc/internal-fn.c   2018-01-09 15:13:32.603106251 +
+++ gcc/internal-fn.c   2018-01-09 15:13:32.785098198 +
@@ -2412,15 +2412,53 @@ expand_LOOP_DIST_ALIAS (internal_fn, gca
   gcc_unreachable ();
 }
 
+/* Return a memory reference of type TYPE for argument INDEX of STMT.
+   Use argument INDEX + 1 to derive the second (TBAA) operand.  */
+
+static tree
+expand_call_mem_ref (tree type, gcall *stmt, int index)
+{
+  tree addr = gimple_call_arg (stmt, index);
+  tree alias_ptr_type = TREE_TYPE (gimple_call_arg (stmt, index + 1));
+  unsigned int align = tree_to_shwi (gimple_call_arg (stmt, index + 1));
+  if (TYPE_ALIGN (type) != align)
+type = build_aligned_type (type, align);
+
+  tree tmp = addr;
+  if (TREE_CODE (tmp) == SSA_NAME)
+{

Re: Make ivopts handle calls to internal functions

[Richard Biener]

On Mon, Nov 20, 2017 at 12:31 PM, Bin.Cheng  wrote:
> On Fri, Nov 17, 2017 at 3:03 PM, Richard Sandiford
>  wrote:
>> ivopts previously treated pointer arguments to internal functions
>> like IFN_MASK_LOAD and IFN_MASK_STORE as normal gimple values.
>> This patch makes it treat them as addresses instead.  This makes
>> a significant difference to the code quality for SVE loops,
>> since we can then use loads and stores with scaled indices.
> Thanks for working on this.  This can be extended to other internal
> functions which eventually
> are expanded into memory references.  I believe (at least) both x86
> and AArch64 has such
> requirement.

In addition to Bins comments I only have a single one (the rest of the
middle-end
changes look OK).  The alias type of MEM_REFs and TARGET_MEM_REFs
in ADDR_EXPR context is meaningless so you don't need to jump through hoops
to get at it or preserve it in any way, likewise for CLIQUE/BASE if it
were present.

Maybe you can simplify code with this.  As you're introducing _MEM_REF
as a valid construct (it weren't before) you'll run into missing /
misguided foldings
eventually.  So be prepared to fix up fallout.

Thanks,
Richard.

>>
>> The patch also adds support for ADDR_EXPRs of TARGET_MEM_REFs,
>> which are the natural way of representing the result of the
>> ivopts transformation.
>>
>> Tested on aarch64-linux-gnu (with and without SVE), x86_64-linux-gnu
>> and powerpc64le-linux-gnu.  OK to install?
>>
>> Richard
>>
>>
>> 2017-11-17  Richard Sandiford  
>> Alan Hayward  
>> David Sherwood  
>>
>> gcc/
>> * expr.c (expand_expr_addr_expr_1): Handle ADDR_EXPRs of
>> TARGET_MEM_REFs.
>> * gimple-expr.h (is_gimple_addressable: Likewise.
>> * gimple-expr.c (is_gimple_address): Likewise.
>> * internal-fn.c (expand_call_mem_ref): New function.
>> (expand_mask_load_optab_fn): Use it.
>> (expand_mask_store_optab_fn): Likewise.
>> * tree-ssa-loop-ivopts.c (USE_ADDRESS): Split into...
>> (USE_REF_ADDRESS, USE_PTR_ADDRESS): ...these new use types.
>> (dump_groups): Update accordingly.
>> (iv_use::mem_type): New member variable.
>> (address_p): New function.
>> (record_use): Add a mem_type argument and initialize the new
>> mem_type field.
>> (record_group_use): Add a mem_type argument.  Use address_p.
>> Update call to record_use.
>> (find_interesting_uses_op): Update call to record_group_use.
>> (find_interesting_uses_cond): Likewise.
>> (find_interesting_uses_address): Likewise.
>> (get_mem_type_for_internal_fn): New function.
>> (find_address_like_use): Likewise.
>> (find_interesting_uses_stmt): Try find_address_like_use before
>> calling find_interesting_uses_op.
>> (addr_offset_valid_p): Use the iv mem_type field as the type
>> of the addressed memory.
>> (add_autoinc_candidates): Likewise.
>> (get_address_cost): Likewise.
>> (split_small_address_groups_p): Use address_p.
>> (split_address_groups): Likewise.
>> (add_iv_candidate_for_use): Likewise.
>> (autoinc_possible_for_pair): Likewise.
>> (rewrite_groups): Likewise.
>> (get_use_type): Check for USE_REF_ADDRESS instead of USE_ADDRESS.
>> (determine_group_iv_cost): Update after split of USE_ADDRESS.
>> (get_alias_ptr_type_for_ptr_address): New function.
>> (rewrite_use_address): Rewrite address uses in calls that were
>> identified by find_address_like_use.
>>
>> gcc/testsuite/
>> * gcc.dg/tree-ssa/scev-9.c: Expected REFERENCE ADDRESS
>> instead of just ADDRESS.
>> * gcc.dg/tree-ssa/scev-10.c: Likewise.
>> * gcc.dg/tree-ssa/scev-11.c: Likewise.
>> * gcc.dg/tree-ssa/scev-12.c: Likewise.
>> * gcc.target/aarch64/sve_index_offset_1.c: New test.
>> * gcc.target/aarch64/sve_index_offset_1_run.c: Likewise.
>> * gcc.target/aarch64/sve_loop_add_2.c: Likewise.
>> * gcc.target/aarch64/sve_loop_add_3.c: Likewise.
>> * gcc.target/aarch64/sve_while_1.c: Check for indexed addressing 
>> modes.
>> * gcc.target/aarch64/sve_while_2.c: Likewise.
>> * gcc.target/aarch64/sve_while_3.c: Likewise.
>> * gcc.target/aarch64/sve_while_4.c: Likewise.
>>
>> Index: gcc/expr.c
>> ===
>> --- gcc/expr.c  2017-11-17 09:49:36.191354637 +
>> +++ gcc/expr.c  2017-11-17 15:02:12.868132458 +
>> @@ -7814,6 +7814,9 @@ expand_expr_addr_expr_1 (tree exp, rtx t
>> return expand_expr (tem, target, tmode, modifier);
>>}
>>
>> +case TARGET_MEM_REF:
>> +  return addr_for_mem_ref (exp, as, true);
>> +
>>  case CONST_DECL:
>> 

Re: Make ivopts handle calls to internal functions

[Bin.Cheng]

On Fri, Nov 17, 2017 at 3:03 PM, Richard Sandiford
 wrote:
> ivopts previously treated pointer arguments to internal functions
> like IFN_MASK_LOAD and IFN_MASK_STORE as normal gimple values.
> This patch makes it treat them as addresses instead.  This makes
> a significant difference to the code quality for SVE loops,
> since we can then use loads and stores with scaled indices.
Thanks for working on this.  This can be extended to other internal
functions which eventually
are expanded into memory references.  I believe (at least) both x86
and AArch64 has such
requirement.

>
> The patch also adds support for ADDR_EXPRs of TARGET_MEM_REFs,
> which are the natural way of representing the result of the
> ivopts transformation.
>
> Tested on aarch64-linux-gnu (with and without SVE), x86_64-linux-gnu
> and powerpc64le-linux-gnu.  OK to install?
>
> Richard
>
>
> 2017-11-17  Richard Sandiford  
> Alan Hayward  
> David Sherwood  
>
> gcc/
> * expr.c (expand_expr_addr_expr_1): Handle ADDR_EXPRs of
> TARGET_MEM_REFs.
> * gimple-expr.h (is_gimple_addressable: Likewise.
> * gimple-expr.c (is_gimple_address): Likewise.
> * internal-fn.c (expand_call_mem_ref): New function.
> (expand_mask_load_optab_fn): Use it.
> (expand_mask_store_optab_fn): Likewise.
> * tree-ssa-loop-ivopts.c (USE_ADDRESS): Split into...
> (USE_REF_ADDRESS, USE_PTR_ADDRESS): ...these new use types.
> (dump_groups): Update accordingly.
> (iv_use::mem_type): New member variable.
> (address_p): New function.
> (record_use): Add a mem_type argument and initialize the new
> mem_type field.
> (record_group_use): Add a mem_type argument.  Use address_p.
> Update call to record_use.
> (find_interesting_uses_op): Update call to record_group_use.
> (find_interesting_uses_cond): Likewise.
> (find_interesting_uses_address): Likewise.
> (get_mem_type_for_internal_fn): New function.
> (find_address_like_use): Likewise.
> (find_interesting_uses_stmt): Try find_address_like_use before
> calling find_interesting_uses_op.
> (addr_offset_valid_p): Use the iv mem_type field as the type
> of the addressed memory.
> (add_autoinc_candidates): Likewise.
> (get_address_cost): Likewise.
> (split_small_address_groups_p): Use address_p.
> (split_address_groups): Likewise.
> (add_iv_candidate_for_use): Likewise.
> (autoinc_possible_for_pair): Likewise.
> (rewrite_groups): Likewise.
> (get_use_type): Check for USE_REF_ADDRESS instead of USE_ADDRESS.
> (determine_group_iv_cost): Update after split of USE_ADDRESS.
> (get_alias_ptr_type_for_ptr_address): New function.
> (rewrite_use_address): Rewrite address uses in calls that were
> identified by find_address_like_use.
>
> gcc/testsuite/
> * gcc.dg/tree-ssa/scev-9.c: Expected REFERENCE ADDRESS
> instead of just ADDRESS.
> * gcc.dg/tree-ssa/scev-10.c: Likewise.
> * gcc.dg/tree-ssa/scev-11.c: Likewise.
> * gcc.dg/tree-ssa/scev-12.c: Likewise.
> * gcc.target/aarch64/sve_index_offset_1.c: New test.
> * gcc.target/aarch64/sve_index_offset_1_run.c: Likewise.
> * gcc.target/aarch64/sve_loop_add_2.c: Likewise.
> * gcc.target/aarch64/sve_loop_add_3.c: Likewise.
> * gcc.target/aarch64/sve_while_1.c: Check for indexed addressing 
> modes.
> * gcc.target/aarch64/sve_while_2.c: Likewise.
> * gcc.target/aarch64/sve_while_3.c: Likewise.
> * gcc.target/aarch64/sve_while_4.c: Likewise.
>
> Index: gcc/expr.c
> ===
> --- gcc/expr.c  2017-11-17 09:49:36.191354637 +
> +++ gcc/expr.c  2017-11-17 15:02:12.868132458 +
> @@ -7814,6 +7814,9 @@ expand_expr_addr_expr_1 (tree exp, rtx t
> return expand_expr (tem, target, tmode, modifier);
>}
>
> +case TARGET_MEM_REF:
> +  return addr_for_mem_ref (exp, as, true);
> +
>  case CONST_DECL:
>/* Expand the initializer like constants above.  */
>result = XEXP (expand_expr_constant (DECL_INITIAL (exp),
> Index: gcc/gimple-expr.h
> ===
> --- gcc/gimple-expr.h   2017-11-17 09:40:43.520567009 +
> +++ gcc/gimple-expr.h   2017-11-17 15:02:12.868132458 +
> @@ -119,6 +119,7 @@ virtual_operand_p (tree op)
>  is_gimple_addressable (tree t)
>  {
>return (is_gimple_id (t) || handled_component_p (t)
> + || TREE_CODE (t) == TARGET_MEM_REF
>   || TREE_CODE (t) == MEM_REF);
>  }
>
> Index: gcc/gimple-expr.c
> ===
> --- gcc/gimple-expr.c