Hi,
Jeff Law <jeffreya...@gmail.com> writes: > On 6/11/23 23:44, Jiufu Guo wrote: >> Richard Biener <rguent...@suse.de> writes: >> >>> On Fri, 9 Jun 2023, Jiufu Guo wrote: >>> >>>> >>>> Hi, >>>> >>>> Richard Biener <rguent...@suse.de> writes: >>>> >>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote: >>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> Richard Biener <rguent...@suse.de> writes: >>>>>> >>>>>>> On Fri, 9 Jun 2023, Richard Sandiford wrote: >>>>>>> >>>>>>>> guojiufu <guoji...@linux.ibm.com> writes: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 2023-06-09 16:00, Richard Biener wrote: >>>>>>>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>> ... >>>>>>>>>>> >>>>>>>>>>> This patch is raised when drafting below one. >>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html. >>>>>>>>>>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into >>>>>>>>>>> try_const_anchors, and hits the assert/ice. >>>>>>>>>>> >>>>>>>>>>> Boostrap and regtest pass on ppc64{,le} and x86_64. >>>>>>>>>>> Is this ok for trunk? >>>>>>>>>> >>>>>>>>>> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then >>>>>>>>>> I suggest to instead fix try_const_anchors to change >>>>>>>>>> >>>>>>>>>> /* CONST_INT is used for CC modes, but we should leave those >>>>>>>>>> alone. >>>>>>>>>> */ >>>>>>>>>> if (GET_MODE_CLASS (mode) == MODE_CC) >>>>>>>>>> return NULL_RTX; >>>>>>>>>> >>>>>>>>>> gcc_assert (SCALAR_INT_MODE_P (mode)); >>>>>>>>>> >>>>>>>>>> to >>>>>>>>>> >>>>>>>>>> /* CONST_INT is used for CC modes, leave any non-scalar-int mode >>>>>>>>>> alone. */ >>>>>>>>>> if (!SCALAR_INT_MODE_P (mode)) >>>>>>>>>> return NULL_RTX; >>>>>>>>>> >>>>>>>>> >>>>>>>>> This is also able to fix this issue. there is a "Punt on CC modes" >>>>>>>>> patch >>>>>>>>> to return NULL_RTX in try_const_anchors. >>>>>>>>> >>>>>>>>>> but as said I wonder how we arrive at a BLKmode CONST_INT and whether >>>>>>>>>> we should have fended this off earlier. Can you share more complete >>>>>>>>>> RTL of that stack_tie? >>>>>>>>> >>>>>>>>> >>>>>>>>> (insn 15 14 16 3 (parallel [ >>>>>>>>> (set (mem/c:BLK (reg/f:DI 1 1) [1 A8]) >>>>>>>>> (const_int 0 [0])) >>>>>>>>> ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie} >>>>>>>>> (nil)) >>>>>>>>> >>>>>>>>> It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". >>>>>>>> >>>>>>>> I'm not convinced this is correct RTL. (unspec:BLK [(const_int 0)] >>>>>>>> ...) >>>>>>>> would be though. It's arguably more accurate too, since the effect >>>>>>>> on the stack locations is unspecified rather than predictable. >>>>>>> >>>>>>> powerpc seems to be the only port with a stack_tie that's not >>>>>>> using an UNSPEC RHS. >>>>>> In rs6000.md, it is >>>>>> >>>>>> ; This is to explain that changes to the stack pointer should >>>>>> ; not be moved over loads from or stores to stack memory. >>>>>> (define_insn "stack_tie" >>>>>> [(match_parallel 0 "tie_operand" >>>>>> [(set (mem:BLK (reg 1)) (const_int 0))])] >>>>>> "" >>>>>> "" >>>>>> [(set_attr "length" "0")]) >>>>>> >>>>>> This would be just an placeholder insn, and acts as the comments. >>>>>> UNSPEC_ would works like other targets. While, I'm wondering >>>>>> the concerns on "set (mem:BLK (reg 1)) (const_int 0)". >>>>>> MODEs between SET_DEST and SET_SRC? >>>>> >>>>> I don't think the issue is the mode but the issue is that >>>>> the patter as-is says some memory is zeroed while that's not >>>>> actually true (not specifying a size means we can't really do >>>>> anything with this MEM, but still). Using an UNSPEC avoids >>>>> implying anything for the stored value. >>>>> >>>>> Of course I think a MEM SET_DEST without a specified size is bougs >>>>> as well, but there's larger precedent for this... >>>> >>>> Thanks for your kindly comments! >>>> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this >>>> insn does not generate real thing (not a real store and no asm code), >>>> may like barrier. >>>> >>>> While I agree that, using UNSPEC may be more clear to avoid mis-reading. >>> >>> Btw, another way to avoid the issue in CSE is to make it not process >>> (aka record anything for optimization) for SET from MEMs with >>> !MEM_SIZE_KNOWN_P >> >> Thanks! Yes, this would make sense. >> Then, there are two ideas(patches) to handle this issue: >> Which one would be preferable? This one (from compiling time aspect)? >> >> And maybe, the changes in rs6000 stack_tie through using unspec >> can be a standalone enhancement besides cse patch. > I'd tend to lean more towards fixing the rs6000 backend. It's basically > lying to the rest of the compiler and when it presents passes with something > like > > (set (mem:BLK) (const_int 0)) > > It's largely inviting the generic bits to treat it like a memory store, when > in fact it's something significantly different. > > I don't think the CSE patch is wrong or a bad idea, more that it's > just papering over a problem caused by an odd chunk of RTL created by > the PPC backend. Thanks a lot for your very kindly and helpful review! Agree with you comments! A patch in rs6000 was prepared to handle this. BR, Jeff (Jiufu Guo) > > jeff
