Re: [PATCH] treat argp-based mem as frame related in dse

[Jeff Law]

On 12/6/23 02:27, Jiufu Guo wrote:
Hi,

The issue mentioned in PR112525 would be able to be handled by
updating dse.cc to treat arg_pointer_rtx similarly with frame_pointer_rtx.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30271#c10 also mentioned
this idea.
                                                                 
One thing, arpg area may be used to pass argument to callee. So, it would
be needed to check if call insns are using that mem.

Bootstrap &regtest pass on ppc64{,le} and x86_64.
Is this ok for trunk?

BR,
Jeff (Jiufu Guo)


        PR rtl-optimization/112525

gcc/ChangeLog:

        * dse.cc (get_group_info): Add arg_pointer_rtx as frame_related.
        (check_mem_read_rtx): Add parameter to indicate if it is checking mem
        for call insn.
        (scan_insn): Add mem checking on call usage.

gcc/testsuite/ChangeLog:

        * gcc.target/powerpc/pr112525.c: New test.
So conceptually the first chunk makes sense.  Though I do worry about 
Andrew's comment about it causing a bootstrap failure.  Even thought it 
was 15 years ago, it remains worrisome.


@@ -2368,7 +2370,8 @@ check_mem_read_rtx (rtx *loc, bb_info_t bb_info)
  
  	  /* If this read is just reading back something that we just
             stored, rewrite the read.  */
-         if (store_info->rhs
+         if (!used_in_call
+             && store_info->rhs
              && store_info->group_id == -1
              && store_info->cse_base == base
              && known_subrange_p (offset, width, store_info->offset,
@@ -2650,6 +2653,12 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int 
max_active_local_stores)
             that is not relative to the frame.  */
          add_non_frame_wild_read (bb_info);
  
+      for (rtx link = CALL_INSN_FUNCTION_USAGE (insn);
+          link != NULL_RTX;
+          link = XEXP (link, 1))
+       if (GET_CODE (XEXP (link, 0)) == USE && MEM_P (XEXP (XEXP (link, 0),0)))
+         check_mem_read_rtx (&XEXP (XEXP (link, 0),0), bb_info, true);
I'm having a bit of a hard time convincing myself this is correct 
though.  I can't see how rewriting the load to read the source of the 
prior store is unsafe.  If that fixes a problem, then it would seem like 
we've gone wrong before here -- perhaps failing to use the fusage loads 
to "kill" any available stores to the same or aliased memory locations.

Assuming we get to a point where we think this or something similar to 
it is safe, then we should retest pr30271 and if it's fixed reference it 
in the ChangeLog.

Jeff