Re: [PATCH] Replace IgnoreSize/DefaultSize with MnemonicSize

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, Mar 3, 2020 at 9:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 03.03.2020 18:15, H.J. Lu wrote:
> > On Tue, Mar 3, 2020 at 6:50 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 03.03.2020 15:09, H.J. Lu wrote:
> >>> I am testing this patch with GCC 8.  I will check it in if it fixes
> >>> regressions in GCC 8 testsuits:
> >>>
> >>> https://gcc.gnu.org/ml/gcc-regression/2020-03/msg00008.html
> >>>
> >>> H.J.
> >>> ---
> >>> According to gas manual, suffix in instruction mnemonics isn't always
> >>> required:
> >>>
> >>> When there is no sizing suffix and no (suitable) register operands to
> >>> deduce the size of memory operands, with a few exceptions and where long
> >>> operand size is possible in the first place, operand size will default
> >>> to long in 32- and 64-bit modes.
> >>
> >> Nothing there says that this defaulting is to happen silently. Yet
> >> _that's_ what my earlier changes altered. The defaulting is still
> >> the same. And no - SUCH CASES SHOULD NOT GO SILENTLY, neither here
> >> nor in the MOVSX/MOVZX case. Ambiguities should _always_ be
> >> pointed out by the assembler. (There may be [and there is] a mode
> >> in which this goes silently, to be enabled at the programmer's
> >> risk.)
> >
> > It is not going to happen in AT&T syntax.   Gas has to support older GCC
> > without any warnings.
>
> Why? What's wrong with pointing out issues even with compiler
> generated code? In fact iirc gcc used to be buggy in regard of
> these conversion instructions, and the assembler change helped
> spot this.

I appreciate your intention.  But the primary goal of gas is to serve GCC.
In this case, there are ino issues with integer conversion in GCC 8 and we
have to support existing GCC 8.  Issue a warning in AT&T syntax is not
really an option here.

> >>> This includes cvtsi2sd, cvtsi2ss, vcvtsi2sd, vcvtsi2ss, vcvtusi2sd and
> >>> vcvtusi2ss.  Since they are used in GCC 8 and older GCC releases, they
> >>> must be allowed without suffix in AT&T syntax.
> >>>
> >>> gas/
> >>>
> >>>       PR gas/25622
> >>>       * testsuite/gas/i386/i386.exp: Run x86-64-default-suffix and
> >>>       x86-64-default-suffix-avx.
> >>>       * testsuite/gas/i386/noreg64.s: Remove cvtsi2sd, cvtsi2ss,
> >>>       vcvtsi2sd, vcvtsi2ss, vcvtusi2sd and vcvtusi2ss entries.
> >>>       * testsuite/gas/i386/noreg64.d: Updated.
> >>>       * testsuite/gas/i386/noreg64.l: Likewise.
> >>>       * testsuite/gas/i386/x86-64-default-suffix-avx.d: New file.
> >>>       * testsuite/gas/i386/x86-64-default-suffix.d: Likewise.
> >>>       * testsuite/gas/i386/x86-64-default-suffix.s: Likewise.
> >>>
> >>> opcodes/
> >>>
> >>>       PR gas/25622
> >>>       * i386-opc.tbl: Add IgnoreSize to cvtsi2sd, cvtsi2ss, vcvtsi2sd,
> >>>       vcvtsi2ss, vcvtusi2sd and vcvtusi2ss for AT&T syntax.
> >>
> >> Oh no. I'm trying to clean up the IgnoreSize mess and you want to
> >> add new instances for no good reason (yes, there are cases where
> >> this is actually missing; hopefully I'll get to send out the
> >> series later this week).
> >
> > Since an instruction template can't have both IgnoreSize and DefaultSize,
> > I am testing this patch and will check it if there are no regressions.  Then
> > we can add one value to MnemonicSize.
>
> It seems pretty unrelated here, but is a good change to make,
> I think.

I checked in in.  Please feel free to replace IgnoreSize on integer conversions
with a new value.

> >> I know I can't prevent this going in, but I'm heavily opposed.
> >> You don't "fix" anything here, you break things.
> >
> > I disagree.
>
> It's pretty sad that in binutils consensus isn't required for
> changes to go in. I'll enter a bug in due course in any event.
>
> Jan


-- 
H.J.