From: Jan Beulich <jbeulich@suse.com>
To: Fangrui Song <i@maskray.me>
Cc: Binutils <binutils@sourceware.org>, Nelson Chu <nelson@rivosinc.com>
Subject: Re: [PATCH 1/3] RISC-V: re-arrange opcode table for consistent alias handling
Date: Wed, 12 Jul 2023 10:15:06 +0200 [thread overview]
Message-ID: <18227b09-0d51-3e24-b782-5cf74e41e494@suse.com> (raw)
In-Reply-To: <DS7PR12MB576580071090C394AECFC618CB31A@DS7PR12MB5765.namprd12.prod.outlook.com>
On 11.07.2023 23:02, Fangrui Song wrote:
> On Fri, Sep 16, 2022 at 2:53 AM Nelson Chu <nelson@rivosinc.com> wrote:
>>
>> On Thu, Sep 15, 2022 at 3:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 15.09.2022 04:30, Nelson Chu wrote:
>>>> On Tue, Sep 13, 2022 at 9:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> --- a/opcodes/riscv-opc.c
>>>>> +++ b/opcodes/riscv-opc.c
>>>>> @@ -290,9 +290,9 @@ const struct riscv_opcode riscv_opcodes[
>>>>> {"jalr", 0, INSN_CLASS_I, "d,s,j", MATCH_JALR, MASK_JALR, match_opcode, INSN_JSR },
>>>>> {"j", 0, INSN_CLASS_C, "Ca", MATCH_C_J, MASK_C_J, match_opcode, INSN_ALIAS|INSN_BRANCH },
>>>>> {"j", 0, INSN_CLASS_I, "a", MATCH_JAL, MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_BRANCH },
>>>>> +{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
>>>>> {"jal", 0, INSN_CLASS_I, "d,a", MATCH_JAL, MASK_JAL, match_opcode, INSN_JSR },
>>>>> {"jal", 32, INSN_CLASS_C, "Ca", MATCH_C_JAL, MASK_C_JAL, match_opcode, INSN_ALIAS|INSN_JSR },
>>>>> -{"jal", 0, INSN_CLASS_I, "a", MATCH_JAL|(X_RA << OP_SH_RD), MASK_JAL|MASK_RD, match_opcode, INSN_ALIAS|INSN_JSR },
>>>>> {"call", 0, INSN_CLASS_I, "d,c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
>>>>> {"call", 0, INSN_CLASS_I, "c", (X_RA << OP_SH_RS1)|(X_RA << OP_SH_RD), (int) M_CALL, match_never, INSN_MACRO },
>>>>> {"tail", 0, INSN_CLASS_I, "c", (X_T1 << OP_SH_RS1), (int) M_CALL, match_never, INSN_MACRO },
>>>>> @@ -310,13 +310,13 @@ const struct riscv_opcode riscv_opcodes[
>>>>> {"move", 0, INSN_CLASS_C, "d,CV", MATCH_C_MV, MASK_C_MV, match_c_add, INSN_ALIAS },
>>>>> {"move", 0, INSN_CLASS_I, "d,s", MATCH_ADDI, MASK_ADDI|MASK_IMM, match_opcode, INSN_ALIAS },
>>>>> {"zext.b", 0, INSN_CLASS_I, "d,s", MATCH_ANDI|ENCODE_ITYPE_IMM (255), MASK_ANDI | MASK_IMM, match_opcode, INSN_ALIAS },
>>>>> -{"andi", 0, INSN_CLASS_C, "Cs,Cw,Co", MATCH_C_ANDI, MASK_C_ANDI, match_opcode, INSN_ALIAS },
>>>>> -{"andi", 0, INSN_CLASS_I, "d,s,j", MATCH_ANDI, MASK_ANDI, match_opcode, 0 },
>>>>> {"and", 0, INSN_CLASS_C, "Cs,Cw,Ct", MATCH_C_AND, MASK_C_AND, match_opcode, INSN_ALIAS },
>>>>> {"and", 0, INSN_CLASS_C, "Cs,Ct,Cw", MATCH_C_AND, MASK_C_AND, match_opcode, INSN_ALIAS },
>>>>> {"and", 0, INSN_CLASS_C, "Cs,Cw,Co", MATCH_C_ANDI, MASK_C_ANDI, match_opcode, INSN_ALIAS },
>>>>> {"and", 0, INSN_CLASS_I, "d,s,t", MATCH_AND, MASK_AND, match_opcode, 0 },
>>>>> {"and", 0, INSN_CLASS_I, "d,s,j", MATCH_ANDI, MASK_ANDI, match_opcode, INSN_ALIAS },
>>>>> +{"andi", 0, INSN_CLASS_C, "Cs,Cw,Co", MATCH_C_ANDI, MASK_C_ANDI, match_opcode, INSN_ALIAS },
>>>>> +{"andi", 0, INSN_CLASS_I, "d,s,j", MATCH_ANDI, MASK_ANDI, match_opcode, 0 },
>>>>
>>>> Doesn't ANDI a base instruction?
>>>
>>> Of course. Like for all aliases, there is a corresponding base
>>> instruction. I guess I simply don't understand what you mean to
>>> express with the question.
>>>
>>>> The operand "d,s,j" of AND is an
>>>> alias of ANDI, so the original order seems correct. Always dump *.i
>>>> instructions to the non-i type looks weird, and llvm-dump seems has
>>>> the same behavior as current GNU objdump.
>>>>
>>>> % cat tmp.s
>>>> and a0, a1, 0x10
>>>> % riscv64-unknown-elf-as tmp.s -o tmp.o
>>>> % riscv64-unknown-elf-objdump -d tmp.o
>>>>
>>>> tmp.o: file format elf64-littleriscv
>>>>
>>>>
>>>> Disassembly of section .text:
>>>>
>>>> 0000000000000000 <.text>:
>>>>
>>>> 0: 0105f513 and a0,a1,16
>>>
>>> What's weird about that? And if that's weird, would you mind spelling
>>> out the conditions under which aliases are to be preferred over base
>>> instructions when disassembling? There actually is a "These aliases are
>>> for assembly but not disassembly" comment somewhere in the file,
>>> clarifying for two of the aliases that they ought to come after their
>>> base insns. But for all other aliases which aren't simply a different
>>> (but not shorter) name for the same insn (e.g. "bgt" vs "blt") I'd
>>> assume the aliases should be preferred, for the reason stated in the
>>> patch description. That said - I can see it being a matter of taste
>>> for <insn>i vs <insn>, but if so this should be spelled out somewhere.
>>
>> Yeah, that's what I worried about. At the beginning, I think dumping
>> a base instruction as another base instruction looks weird. But these
>> days I also noticed that - we also dump compressed instructions as
>> base i without "c." prefixes, so why I feel weird is just that I'm
>> used to it because of historical behavior. I have no objection to
>> this, so please go ahead if there are no objections for a period of
>> time. But if there are any objections, then we probably can mark
>> these aliases by something like INSN_ALIAS_CANNOT_DUMP in the opcode
>> table, and that's what Kito suggested to me before, but I didn't think
>> it was a serious problem at the time.
>>
>> Thanks
>> Nelson
>
> I apologize as I haven't read all prior discussions. For many
> instructions, the "i" form is written in the ISA manual and prevalent.
Why "prevalent"? The "i"-less forms are mentioned there as well, aren't
they? Then why not use them ...
> I wonder whether we can give these add/and/xor/etc without "i" lower
> priority so that objdump -d will not show them, even without using -M
> no-aliases.
... unless use of aliases was suppressed? In other arches' assembly
that I know (to some degree) and which knows the concept of aliases,
aliases are typically the preferred way of disassembling, for
typically producing easier to grok output.
There are other aspects to consider here, related to the handling of
equates in assembly sources. I did bring this up before, but it feels
like a minefield - it first would need firmly establishing what exactly
assembler behavior is intended to be. Aiui no-one has properly thought
of this, including for the (surprisingly similar) handling in tc-mips.c
(making me guess that RISC-V code may have been derived from that).
Jan
> % cat b.s
> add a0,a1,13
> and a2,a3,4
> xor a2,a3,4
> or a2,a3,4
> sll a2,a3,4
> % riscv64-linux-gnu-gcc -c b.s
> % ~/Dev/binutils-gdb/out/riscv64/binutils/objdump -d b.o
>
> b.o: file format elf64-littleriscv
>
>
> Disassembly of section .text:
>
> 0000000000000000 <.text>:
> 0: 00d58513 add a0,a1,13
> 4: 0046f613 and a2,a3,4
> 8: 0046c613 xor a2,a3,4
> c: 0046e613 or a2,a3,4
> 10: 00469613 sll a2,a3,0x4
>
>
> When LLVM integrated assembler added these aliases
> (https://reviews.llvm.org/D50046), these instructions are assigned a
> low priority "let EmitPriority = 0" so llvm-objdump -d will never show
> them.
next prev parent reply other threads:[~2023-07-12 8:15 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-13 12:59 [PATCH 0/3] RISC-V: alias insn adjustments Jan Beulich
2022-09-13 13:02 ` [PATCH 1/3] RISC-V: re-arrange opcode table for consistent alias handling Jan Beulich
2022-09-15 2:30 ` Nelson Chu
2022-09-15 7:42 ` Jan Beulich
2022-09-16 9:53 ` Nelson Chu
2023-07-11 21:02 ` Fangrui Song
[not found] ` <DS7PR12MB576580071090C394AECFC618CB31A@DS7PR12MB5765.namprd12.prod.outlook.com>
2023-07-12 8:15 ` Jan Beulich [this message]
2023-07-14 21:25 ` Fangrui Song
[not found] ` <MN0PR12MB57613B59178FAADE5FCD3837CB34A@MN0PR12MB5761.namprd12.prod.outlook.com>
2023-07-14 22:08 ` Stefan O'Rear
2023-07-17 6:50 ` Jan Beulich
2023-07-21 22:16 ` Song Fangrui
2023-07-22 15:14 ` Jeff Law
2023-07-22 16:55 ` Andrew Waterman
[not found] ` <DS7PR12MB57658EF28577B41BF35C5E7CCB3FA@DS7PR12MB5765.namprd12.prod.outlook.com>
2023-07-24 7:23 ` Jan Beulich
2023-08-30 3:14 ` Fangrui Song
2022-09-13 13:03 ` [PATCH 2/3] RISC-V: drop stray INSN_ALIAS flags Jan Beulich
2022-09-15 2:43 ` Nelson Chu
2022-09-13 13:04 ` [PATCH 3/3] RISC-V: add alias for SLLI.UW Jan Beulich
2022-09-13 14:54 ` [PATCH 0/3] RISC-V: alias insn adjustments Tsukasa OI
2022-09-13 16:11 ` Jan Beulich
2022-09-13 16:58 ` Tsukasa OI
2022-09-14 6:26 ` Jan Beulich
2022-09-30 9:41 ` [PATCH] RISC-V: fix build after "Add support for arbitrary immediate encoding formats" Jan Beulich
2022-09-30 10:26 ` Christoph Müllner
2022-09-30 22:17 ` Palmer Dabbelt
2022-09-30 9:42 ` [PATCH] RISC-V: fallout from "re-arrange opcode table for consistent alias handling" Jan Beulich
2022-09-30 9:42 ` [PATCH] RISC-V: don't cast expressions' X_add_number to long in diagnostics Jan Beulich
2022-09-30 10:13 ` Christoph Müllner
2022-09-30 14:43 ` Nelson Chu
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=18227b09-0d51-3e24-b782-5cf74e41e494@suse.com \
--to=jbeulich@suse.com \
--cc=binutils@sourceware.org \
--cc=i@maskray.me \
--cc=nelson@rivosinc.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).