diff mbox series

AutoFDO: Walk predecessor edges for PHI arguments

Message ID 20260804003921.90054-1-kvivekananda@nvidia.com
State Committed
Commit 01cbfb28f7140f5a6089151b98bdf6a15532fda6
Headers
Series AutoFDO: Walk predecessor edges for PHI arguments |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed

Commit Message

Kugan Vivekanandarajah Aug. 4, 2026, 12:39 a.m. UTC 
  PHI arguments are indexed by incoming edges.  function_instance::match
walked successor edges and could fail gimple_phi_arg_def_from_edge's
destination assertion during afdo_offline.

gcc/ChangeLog:

	* auto-profile.cc (function_instance::match): Walk predecessor edges
	for PHI arguments.

Regression tested on aarch64-linux-gnu with no new regressions

Signed-off-by: Kugan Vivekanandarajah <kvivekananda@nvidia.com>
---
 gcc/auto-profile.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jan Hubička Aug. 4, 2026, 6:43 a.m. UTC | #1 
On Tue, Aug 4, 2026 at 2:39 AM Kugan Vivekanandarajah <
kvivekananda@nvidia.com> wrote:

> PHI arguments are indexed by incoming edges.  function_instance::match
> walked successor edges and could fail gimple_phi_arg_def_from_edge's
> destination assertion during afdo_offline.
>
> gcc/ChangeLog:
>
>         * auto-profile.cc (function_instance::match): Walk predecessor
> edges
>         for PHI arguments.
>

This is OK.  It is really strange this does not trigger checking failure
I see that
inline location_t
gimple_phi_arg_location_from_edge (gphi *phi, edge e)
{
 return gimple_phi_arg (phi, e->dest_idx)->locus;

Does not check that PHI BB and e->dest match (that looks like omission that
would be useful to fix), but still we should eventually hit out of range
access which is checked in gimple_phi_arg.

Honza

>
> Regression tested on aarch64-linux-gnu with no new regressions
>
> Signed-off-by: Kugan Vivekanandarajah <kvivekananda@nvidia.com>
> ---
>  gcc/auto-profile.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
> index f4562d7f2f98..60f6b1ba480b 100644
> --- a/gcc/auto-profile.cc
> +++ b/gcc/auto-profile.cc
> @@ -1764,7 +1764,8 @@ function_instance::match (cgraph_node *node,
>           gcc_assert (!info);
>           dump_stmt (phi, info, NULL, stack);
>           counts.add (info);
> -         for (edge e : bb->succs)
> +         /* PHI arguments are indexed by incoming (predecessor) edges.  */
> +         for (edge e : bb->preds)
>             {
>               location_t phi_loc
>                 = gimple_phi_arg_location_from_edge (phi, e);
> --
> 2.34.1
>
>
Richard Biener Aug. 4, 2026, 7:06 a.m. UTC | #2 
On Tue, Aug 4, 2026 at 8:44 AM Jan Hubička <honza.hubicka@gmail.com> wrote:
>
>
>
> On Tue, Aug 4, 2026 at 2:39 AM Kugan Vivekanandarajah <kvivekananda@nvidia.com> wrote:
>>
>> PHI arguments are indexed by incoming edges.  function_instance::match
>> walked successor edges and could fail gimple_phi_arg_def_from_edge's
>> destination assertion during afdo_offline.
>>
>> gcc/ChangeLog:
>>
>>         * auto-profile.cc (function_instance::match): Walk predecessor edges
>>         for PHI arguments.
>
>
> This is OK.  It is really strange this does not trigger checking failure
> I see that
> inline location_t
> gimple_phi_arg_location_from_edge (gphi *phi, edge e)
> {
>  return gimple_phi_arg (phi, e->dest_idx)->locus;
>
> Does not check that PHI BB and e->dest match (that looks like omission that would be useful to fix), but still we should eventually hit out of range access which is checked in gimple_phi_arg.

I have put such checking in some of the accessors (because as you guess,
it's "easy" to feed bogus edges into those).  gimple_phi_arg_def_from_edge
it seems.  Similar checking assert would be nice to have in other edge based
PHI accessors.

Richard.

>
> Honza
>>
>>
>> Regression tested on aarch64-linux-gnu with no new regressions
>>
>> Signed-off-by: Kugan Vivekanandarajah <kvivekananda@nvidia.com>
>> ---
>>  gcc/auto-profile.cc | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
>> index f4562d7f2f98..60f6b1ba480b 100644
>> --- a/gcc/auto-profile.cc
>> +++ b/gcc/auto-profile.cc
>> @@ -1764,7 +1764,8 @@ function_instance::match (cgraph_node *node,
>>           gcc_assert (!info);
>>           dump_stmt (phi, info, NULL, stack);
>>           counts.add (info);
>> -         for (edge e : bb->succs)
>> +         /* PHI arguments are indexed by incoming (predecessor) edges.  */
>> +         for (edge e : bb->preds)
>>             {
>>               location_t phi_loc
>>                 = gimple_phi_arg_location_from_edge (phi, e);
>> --
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
index f4562d7f2f98..60f6b1ba480b 100644
--- a/gcc/auto-profile.cc
+++ b/gcc/auto-profile.cc
@@ -1764,7 +1764,8 @@  function_instance::match (cgraph_node *node,
 	  gcc_assert (!info);
 	  dump_stmt (phi, info, NULL, stack);
 	  counts.add (info);
-	  for (edge e : bb->succs)
+	  /* PHI arguments are indexed by incoming (predecessor) edges.  */
+	  for (edge e : bb->preds)
 	    {
 	      location_t phi_loc
 		= gimple_phi_arg_location_from_edge (phi, e);