RE: [PATCH, MIPS] Target flag and build option to disable indexed memory OPs.

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

> -----Original Message-----
> From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com]
> Sent: Tuesday, January 17, 2017 4:35 AM
> To: Moore, Catherine <Catherine_Moore@mentor.com>; Doug
> Gilmore <Doug.Gilmore@imgtec.com>; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH, MIPS] Target flag and build option to disable
> indexed memory OPs.
> 
> Moore, Catherine <Catherine_Moore@mentor.com> writes:
> > > -----Original Message-----
> > > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> > > owner@gcc.gnu.org] On Behalf Of Matthew Fortune
> > > Sent: Monday, January 16, 2017 11:25 AM
> > > To: Doug Gilmore <Doug.Gilmore@imgtec.com>; gcc-
> > > patches@gcc.gnu.org
> > > Cc: Moore, Catherine <Catherine_Moore@mentor.com>
> > > Subject: RE: [PATCH, MIPS] Target flag and build option to disable
> > > indexed memory OPs.
> > >
> > > Doug Gilmore <Doug.Gilmore@imgtec.com>
> > > > I recently bisected PR78176 to problems introduced with r21650.
> > > >
> > > > Given the short time until the release, we would like to provide a
> > > > target flag and build option to avoid the bug until we are able to
> > > > resolve the problem with the commit.  Note that as Matthew
> Fortune
> > > has
> > > > mentioned in the PR:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78176#c5
> > > >
> > > > the problem could also be addressed by updates to the Linux
> kernel
> > > since
> > > > the problem is only exposed by running MIPS 32-bit binaries on
> 64-
> > > bit
> > > > kernels.
> > > >
> > > > Bootstrapped on X86_64, regression tested on X86_64 and MIPS.
> > > >
> > > > OK to commit?
> > >
> > > Given this is a generic reference to indexed load/store and the
> issue
> > > could
> > > affect any indexed operation then I think it needs to include all of
> the
> > > following as well:
> > >
> > > /* ISA has lwxs instruction (load w/scaled index address.  */
> > > #define ISA_HAS_LWXS            ((TARGET_SMARTMIPS ||
> > > TARGET_MICROMIPS) \
> > >                                  && !TARGET_MIPS16)
> > >
> > > /* ISA has lbx, lbux, lhx, lhx, lhux, lwx, lwux, or ldx instruction. */
> > > #define ISA_HAS_LBX             (TARGET_OCTEON2)
> > > #define ISA_HAS_LBUX            (ISA_HAS_DSP || TARGET_OCTEON2)
> > > #define ISA_HAS_LHX             (ISA_HAS_DSP || TARGET_OCTEON2)
> > > #define ISA_HAS_LHUX            (TARGET_OCTEON2)
> > > #define ISA_HAS_LWX             (ISA_HAS_DSP || TARGET_OCTEON2)
> > > #define ISA_HAS_LWUX            (TARGET_OCTEON2 &&
> TARGET_64BIT)
> > > #define ISA_HAS_LDX             ((ISA_HAS_DSP || TARGET_OCTEON2)
> \
> > >                                  && TARGET_64BIT)
> > >
> > > The DSP LBUX/LHX/LWX/LDX intrinsics will also need a new AVAIL
> > > predicate
> > > to disable them. The snag is that some DSP code will fail to compile
> if it
> > > uses the DSP load intrinsics directly.
> > >
> > > I see no way of avoiding that. Therefore, distributions that use
> > > --without-indexed-load-store will have to cope with some potential
> > > DSP
> > > fallout if they enable DSP at all.
> > >
> > > @Catherine: I'd like your input here if possible as I advocated this
> > > approach, comments on option names welcome too.  I quite like
> the
> > > verbose
> > > name.
> >
> > Okay, based on my reading of the comments in the bug report, you
> are proposing this option
> > as a workaround to a kernel deficiency.  I don't see any agreement
> that this is actually a
> > compiler bug.
> > Do we really need to include the DSP instrinsics as well?   Do you
> think that many
> > distributions actually enable DSP?
> >
> > The option name itself is acceptable to me.  I'd like to see
> documentation that explains
> > when this problem is exposed.  I'd like to limit the fix to LWXS and I'd
> like to see the
> > testcase from the bug report added to the testsuite.
> > I also agree that the preprocessor macro is a good idea (even if we
> decide to forgo the
> > DSP portion of the patch).
> 
> Thanks for the comments.
> 
> Having thought further I agree we can safely ignore DSP indexed load
> and micromips LWXS on
> the basis that DSP code will not run on a MIPS64 processor anyway (at
> least none that I
> know of) so the issue cannot occur and similarly for microMIPS, there
> are no 64-bit cores.
> 
> Restricting to just LWXC1/SWXC1/LDXC1/SDXC1 is therefore fine but
> we should reflect
> that in option names then.
> 
> --with-lxc1-sxc1 --without-lxc1-sxc1
> -mlxc1-sxc1
> 
> These names reflect the internal macro that controls availability of
> these instructions.
> 
> Macro name: __mips_no_lxc1_sxc1
> Defined when !ISA_HAS_LXC1_SXC1 so would be present even when
> targeting a core that
> doesn't have the instructions anyway.
> 
> Any refinements to this Catherine?
> 
No.  This plan looks good.