On Thu, Jun 1, 2023 at 7:36 AM Jeff Law <jeffreya...@gmail.com> wrote: > > > > On 5/31/23 15:22, Andrew Pinski wrote: > > On Wed, May 31, 2023 at 12:29 AM Richard Biener via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > >> > >> On Wed, May 31, 2023 at 6:34 AM Andrew Pinski via Gcc-patches > >> <gcc-patches@gcc.gnu.org> wrote: > >>> > >>> After r14-1014-gc5df248509b489364c573e8, GCC started to emit > >>> directly a zero_extract for `(t1&0x8)!=0`. This introduced > >>> a small regression where ifcvt would not do the ifconversion > >>> as there is now a paradoxical subreg in the dest which > >>> was being rejected. Since paradoxical subreg set the whole > >>> register, we can treat it as the same as a reg in the two places. > >>> > >>> OK? Bootstrapped and tested on x86_64-linux-gnu and aarch64-linux-gnu. > >> > >> OK I guess. I vaguely remember SUBREG_PROMOTED_UNSIGNED_P > >> applies to non-paradoxical subregs but I might be swapping things - maybe > >> you remember better and whether that would cause any issues here? > > > > So I looked into the history of the code in ifcvt.cc, this code was > > added with r6-3071-ge65bf4e814d38c to accept more complex bb > > (https://inbox.sourceware.org/gcc-patches/559fbb13.80...@arm.com/). > > The thread where we start talking about subregs is located with Jeff's > > email starting here: > > https://inbox.sourceware.org/gcc-patches/55bbafac.5020...@redhat.com/ . > > > > Jeff, > > I know Richard already approved this patch but could you provide a > > second eye as you were involved reviewing the original code here and I > > want to make sure I understood the code in a a reasonable fashion? > It's been a while. I think my original concerns were with RMW operands > and making sure we tracked all sub-components of such operands correctly. > > That was based on the original version, but that version looks like it > should have been OK after reviewing the details of reg_referenced_p. > It's since moved to DF. > > So the only worry I immediately see is whether or not DF is giving uses > and sets of sub-compenents of a RMW operand or multi-hard register modes.
So I looked into the code some more, for bb_valid_for_noce_process_p, we are checking to make sure if a reg that was being set is not used by the comparison (and it works using reg_overlap_mentioned_p that satisfies the multi-hard register mode use case and satisfies the RMW case as we are just checking to make sure it does not do that to the registers of the comparison). For bbs_ok_for_cmove_arith, having the check as paradoxical_subreg_p (rather than a plain SUBREG) satisfies RMW operand case (as it is not a RMW operation) and DF already handles multi-hard register when using FOR_EACH_INSN_DEF/FOR_EACH_INSN_USE (and a paradoxical hard register is an invalid RTL in the first place). I wrote this up more to convince myself this is safe and for future references for others when looking into the code to understand it. Thanks, Andrew > > Jeff
