From: Jan Beulich <jbeulich@suse.com>
To: "H.J. Lu" <hjl.tools@gmail.com>
Cc: Binutils <binutils@sourceware.org>
Subject: Re: [PATCH] Replace IgnoreSize/DefaultSize with MnemonicSize
Date: Wed, 04 Mar 2020 16:24:00 -0000 [thread overview]
Message-ID: <127bebb1-0ae7-593e-695b-13c0cb50e9f6@suse.com> (raw)
In-Reply-To: <CAMe9rOpvLGJbN6SrSxM9O7Wom2PssMG-W6Gtmb5_2+3t+=fkAw@mail.gmail.com>
On 03.03.2020 20:31, H.J. Lu wrote:
> 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.
The initial release of gcc 8 was still buggy, and you can then
extrapolate this to older versions. If code is to be compiled
warning-free with older releases, gcc should get respective
backports. Hiding actual bugs (because of gcc shortcomings) is
not really an option here (to use your wording).
Furthermore - if you were to discover more problems with even
older gcc versions, would you then "declare" all those insns
as exceptions too? Such an approach doesn't make any sense to
me.
>>>>> 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.
As per the series sent earlier today - we're not quite there yet.
Jan
next prev parent reply other threads:[~2020-03-04 16:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-03 14:09 [PATCH] x86: Allow integer conversion without suffix in AT&T syntax H.J. Lu
2020-03-03 14:50 ` Jan Beulich
2020-03-03 17:15 ` [PATCH] Replace IgnoreSize/DefaultSize with MnemonicSize H.J. Lu
2020-03-03 17:26 ` Jan Beulich
2020-03-03 19:32 ` H.J. Lu
2020-03-04 16:24 ` Jan Beulich [this message]
2020-03-04 17:49 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=127bebb1-0ae7-593e-695b-13c0cb50e9f6@suse.com \
--to=jbeulich@suse.com \
--cc=binutils@sourceware.org \
--cc=hjl.tools@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).