[10/12] mode-switching: Use 1-based edge aux fields

  The pass used the edge aux field to record which mode change
should happen on the edge, with -1 meaning "none".  It's more
convenient for later patches to leave aux zero for "none",
and use numbers based at 1 to record a change.

gcc/
	* mode-switching.cc (commit_mode_sets): Use 1-based edge aux values.
---
 gcc/mode-switching.cc | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
  

On 11/5/23 11:49, Richard Sandiford wrote:
> The pass used the edge aux field to record which mode change
> should happen on the edge, with -1 meaning "none".  It's more
> convenient for later patches to leave aux zero for "none",
> and use numbers based at 1 to record a change.
> 
> gcc/
> 	* mode-switching.cc (commit_mode_sets): Use 1-based edge aux values.
So my only worry here is the state of the aux field as we enter mode 
switching.  ISTM the old code would never depend on that previous state 
since it always initialized eg->aux to -1, then conditionally overwrote 
that value if there was an insertion.  Then it could clear all the aux 
fields once a particular entity was resolved.

It appears now that for the first entity we depend on the aux field 
being clear as we enter mode switching.  Or am I missing something?

jeff
  

Jeff Law <jeffreyalaw@gmail.com> writes:
> On 11/5/23 11:49, Richard Sandiford wrote:
>> The pass used the edge aux field to record which mode change
>> should happen on the edge, with -1 meaning "none".  It's more
>> convenient for later patches to leave aux zero for "none",
>> and use numbers based at 1 to record a change.
>> 
>> gcc/
>> 	* mode-switching.cc (commit_mode_sets): Use 1-based edge aux values.
> So my only worry here is the state of the aux field as we enter mode 
> switching.  ISTM the old code would never depend on that previous state 
> since it always initialized eg->aux to -1, then conditionally overwrote 
> that value if there was an insertion.  Then it could clear all the aux 
> fields once a particular entity was resolved.
>
> It appears now that for the first entity we depend on the aux field 
> being clear as we enter mode switching.  Or am I missing something?

Yeah, we'd rely on that.  12/12 would too for all edges.

I could have sworn that there was something that checked that passes
left edge aux fields clear, but it looks like I misremembered.  So I
probably need to stick a clear_aux_for_edges () call above the first
main loop (for 12/12) and keep the initialisation here as well.

That kind-of takes away the point of shifting to 1-based values
in the first place.  Ah well...

Thanks,
Richard
  

On 11/7/23 17:35, Richard Sandiford wrote:

> I could have sworn that there was something that checked that passes
> left edge aux fields clear, but it looks like I misremembered.  So I
> probably need to stick a clear_aux_for_edges () call above the first
> main loop (for 12/12) and keep the initialisation here as well.
That does sound vaguely familiar.   Maybe it was a one-off test someone 
did.

> 
> That kind-of takes away the point of shifting to 1-based values
> in the first place.  Ah well...
Your call.  I'd tend to lean towards inserting the clear_aux call if we 
don't have something that's consistently verifying aux state. 
Alternately we can return to the -1 handling.  I doubt it's all that 
important from a compile-time standpoint.

jeff
  

Jeff Law <jlaw@ventanamicro.com> writes:
> On 11/7/23 17:35, Richard Sandiford wrote:
>
>> I could have sworn that there was something that checked that passes
>> left edge aux fields clear, but it looks like I misremembered.  So I
>> probably need to stick a clear_aux_for_edges () call above the first
>> main loop (for 12/12) and keep the initialisation here as well.
> That does sound vaguely familiar.   Maybe it was a one-off test someone 
> did.
>
>> 
>> That kind-of takes away the point of shifting to 1-based values
>> in the first place.  Ah well...
> Your call.  I'd tend to lean towards inserting the clear_aux call if we 
> don't have something that's consistently verifying aux state. 

Agreed.  A convention that isn't tested isn't strong enough to rely on.

> Alternately we can return to the -1 handling.  I doubt it's all that 
> important from a compile-time standpoint.

I dithered about it a bit, but in the end, zero does still seem to work
out a bit more nicely.

I think you'd basically OKed the patch with this change, but I'm feeling
extra risk-averse at the moment :) so, here's an updated patch.
Tested as before.  OK to install?

Thanks for catching this.

Richard


gcc/
	* mode-switching.cc (commit_mode_sets): Use 1-based edge aux values.
---
 gcc/mode-switching.cc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/mode-switching.cc b/gcc/mode-switching.cc
index 7a5c4993d65..6b5661131e3 100644
--- a/gcc/mode-switching.cc
+++ b/gcc/mode-switching.cc
@@ -106,10 +106,10 @@ commit_mode_sets (struct edge_list *edge_list, int e, struct bb_info *info)
   for (int ed = NUM_EDGES (edge_list) - 1; ed >= 0; ed--)
     {
       edge eg = INDEX_EDGE (edge_list, ed);
-      int mode;
 
-      if ((mode = (int)(intptr_t)(eg->aux)) != -1)
+      if (eg->aux)
 	{
+	  int mode = (int) (intptr_t) eg->aux - 1;
 	  HARD_REG_SET live_at_edge;
 	  basic_block src_bb = eg->src;
 	  int cur_mode = info[src_bb->index].mode_out;
@@ -728,14 +728,14 @@ optimize_mode_switching (void)
 	{
 	  edge eg = INDEX_EDGE (edge_list, ed);
 
-	  eg->aux = (void *)(intptr_t)-1;
+	  eg->aux = (void *) (intptr_t) 0;
 
 	  for (i = 0; i < no_mode; i++)
 	    {
 	      int m = targetm.mode_switching.priority (entity_map[j], i);
 	      if (mode_bit_p (insert[ed], j, m))
 		{
-		  eg->aux = (void *)(intptr_t)m;
+		  eg->aux = (void *) (intptr_t) (m + 1);
 		  break;
 		}
 	    }
  

On 11/11/23 08:51, Richard Sandiford wrote:
> Jeff Law <jlaw@ventanamicro.com> writes:
>> On 11/7/23 17:35, Richard Sandiford wrote:
>>
>>> I could have sworn that there was something that checked that passes
>>> left edge aux fields clear, but it looks like I misremembered.  So I
>>> probably need to stick a clear_aux_for_edges () call above the first
>>> main loop (for 12/12) and keep the initialisation here as well.
>> That does sound vaguely familiar.   Maybe it was a one-off test someone
>> did.
>>
>>>
>>> That kind-of takes away the point of shifting to 1-based values
>>> in the first place.  Ah well...
>> Your call.  I'd tend to lean towards inserting the clear_aux call if we
>> don't have something that's consistently verifying aux state.
> 
> Agreed.  A convention that isn't tested isn't strong enough to rely on.
> 
>> Alternately we can return to the -1 handling.  I doubt it's all that
>> important from a compile-time standpoint.
> 
> I dithered about it a bit, but in the end, zero does still seem to work
> out a bit more nicely.
> 
> I think you'd basically OKed the patch with this change, but I'm feeling
> extra risk-averse at the moment :) so, here's an updated patch.
> Tested as before.  OK to install?
> 
> Thanks for catching this.
> 
> Richard
> 
> 
> gcc/
> 	* mode-switching.cc (commit_mode_sets): Use 1-based edge aux values.
Yea, intention was to ACK whichever approach you preferred.

OK.

jeff