[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format #74053

MouseSplinter · 2023-12-01T09:11:18Z

Enable the llvm-objdump to disassemble the immediate of RISCV instruction in hexadecimal format with --print-imm-hex flag.

llvmbot · 2023-12-01T09:11:49Z

@llvm/pr-subscribers-lld-elf
@llvm/pr-subscribers-lld
@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-mc

Author: Wang Yaduo (MouseSplinter)

Changes

Enable the llvm-objdump to disassemble the immediate of RISCV instruction in hexadecimal format with --print-imm-hex flag.

Full diff: https://github.com/llvm/llvm-project/pull/74053.diff

2 Files Affected:

(modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp (+4-4)
(added) llvm/test/MC/RISCV/print-imm-hex.s (+42)

diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
index a6f3f7f8d18e069..195dda0b8b14059 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
@@ -91,7 +91,7 @@ void RISCVInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
   }
 
   if (MO.isImm()) {
-    markup(O, Markup::Immediate) << MO.getImm();
+    markup(O, Markup::Immediate) << formatImm(MO.getImm());
     return;
   }
 
@@ -113,7 +113,7 @@ void RISCVInstPrinter::printBranchOperand(const MCInst *MI, uint64_t Address,
       Target &= 0xffffffff;
     markup(O, Markup::Target) << formatHex(Target);
   } else {
-    markup(O, Markup::Target) << MO.getImm();
+    markup(O, Markup::Target) << formatImm(MO.getImm());
   }
 }
 
@@ -128,7 +128,7 @@ void RISCVInstPrinter::printCSRSystemRegister(const MCInst *MI, unsigned OpNo,
   else if (SysReg && SysReg->haveRequiredFeatures(STI.getFeatureBits()))
     markup(O, Markup::Register) << SysReg->Name;
   else
-    markup(O, Markup::Register) << Imm;
+    markup(O, Markup::Register) << formatImm(Imm);
 }
 
 void RISCVInstPrinter::printFenceArg(const MCInst *MI, unsigned OpNo,
@@ -212,7 +212,7 @@ void RISCVInstPrinter::printVTypeI(const MCInst *MI, unsigned OpNo,
   // or non-zero in bits 8 and above.
   if (RISCVVType::getVLMUL(Imm) == RISCVII::VLMUL::LMUL_RESERVED ||
       RISCVVType::getSEW(Imm) > 64 || (Imm >> 8) != 0) {
-    O << Imm;
+    O << formatImm(Imm);
     return;
   }
   // Print the text form.
diff --git a/llvm/test/MC/RISCV/print-imm-hex.s b/llvm/test/MC/RISCV/print-imm-hex.s
new file mode 100644
index 000000000000000..dd35b5c423a9d15
--- /dev/null
+++ b/llvm/test/MC/RISCV/print-imm-hex.s
@@ -0,0 +1,42 @@
+# RUN: llvm-mc %s -triple=riscv32 -riscv-no-aliases -show-encoding -mattr=+v \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM %s
+# RUN: llvm-mc %s -triple=riscv32 -riscv-no-aliases -show-encoding -mattr=+v --print-imm-hex \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM-HEX %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+v < %s \
+# RUN:     | llvm-objdump -M no-aliases --mattr=+v -d -r - \
+# RUN:     | FileCheck -check-prefixes=CHECK-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+v < %s \
+# RUN:     | llvm-objdump -M no-aliases --mattr=+v --print-imm-hex -d -r - \
+# RUN:     | FileCheck -check-prefixes=CHECK-OBJ-HEX %s
+
+# CHECK-ASM: beq s1, s1, 102
+# CHECK-ASM-HEX: beq s1, s1, 0x66
+# CHECK-OBJ: beq s1, s1, 0x66
+# CHECK-OBJ-HEX: beq s1, s1, 0x66
+beq s1, s1, 102
+
+_sym:
+# CHECK-ASM: beq s1, s1, _sym
+# CHECK-ASM-HEX: beq s1, s1, _sym
+# CHECK-OBJ: beq s1, s1, 0x4
+# CHECK-OBJ-HEX: beq s1, s1, 0x4
+beq s1, s1, _sym
+
+# CHECK-ASM: lw a0, 97(a2)
+# CHECK-ASM-HEX: lw a0, 0x61(a2)
+# CHECK-OBJ: lw a0, 97(a2)
+# CHECK-OBJ-HEX: lw a0, 0x61(a2)
+lw a0, 97(a2)
+
+# CHECK-ASM: csrrwi t0, 4095, 31
+# CHECK-ASM-HEX: csrrwi t0, 0xfff, 0x1f
+# CHECK-OBJ: csrrwi t0, 4095, 31
+# CHECK-OBJ-HEX: csrrwi t0, 0xfff, 0x1f
+csrrwi t0, 0xfff, 31
+
+
+# CHECK-ASM: vsetvli a2, a0, 255
+# CHECK-ASM-HEX: vsetvli a2, a0, 0xff
+# CHECK-OBJ: vsetvli a2, a0, 255
+# CHECK-OBJ-HEX: vsetvli a2, a0, 0xff
+vsetvli a2, a0, 0xff

wangpc-pp

LGTM! One question: is this behavior the same as current GNU toolchain?

MouseSplinter · 2023-12-05T08:35:06Z

Yes, GNU objdump print the immediates in hexadecimal format by default, and the llvm-objdump will do so with this patch. :)

wangpc-pp · 2023-12-05T08:53:15Z

Then I have no question now, please go ahead and merge this PR. :-)

Enable the llvm-objdump to disassemble the immediate of RISCV instruction in hexadecimal format with --print-imm-hex flag.

topperc · 2023-12-06T00:17:44Z

gnu objdump for RISC-V is inconsistent. Looking at a dump from 2.41 I see
Shift by immediate uses hex.
addi/andi/ori/xori/li use decimal
lui uses hex.
load/store offset use decimal.
label offsets on branches use hex
vmv.v.i uses decimal

MouseSplinter · 2023-12-06T02:27:55Z

Thanks for your correction, and do you think we should conform to gnu's behavior? Or just be the same as other architecture in llvm.

topperc · 2023-12-06T02:45:09Z

We can conform to other architectures.

topperc

LGTM

MouseSplinter · 2023-12-06T02:54:15Z

Thanks.

asb

The proposed change LGTM, thanks.

vitalybuka · 2023-12-15T04:43:51Z

Fails here https://lab.llvm.org/buildbot/#/builders/5/builds/39291/steps/9/logs/stdio

dyung · 2023-12-15T05:16:32Z

Also failing on https://lab.llvm.org/buildbot/#/builders/247/builds/12248 and https://lab.llvm.org/buildbot/#/builders/275/builds/2345

…imal format (#74053)" This reverts commit 3dde0d0.

…imal format" (#75561) Reverts #74053 Breaks https://lab.llvm.org/buildbot/#/builders/5/builds/39291 Co-authored-by: Wang Yaduo <wangyaduo@linux.alibaba.com> Issue #75563

MaskRay · 2023-12-15T06:38:13Z

LGTM

When merging a change, especially with a lot of test updates, always ensure that the rebased version still works...

Sometimes, only through rebasing can one notice that the patch needs adjustments to more code or tests due to its interaction with another landed patch:

Another landed patch added some tests that would be changed by the current patch.
Another landed patch added a new use of the function that is renamed by the current patch.

--

You can rebase locally and test, but not push an amended commit to the branch, then use the github to merge.

…mat (#74053) Enable the llvm-objdump to disassemble the immediate of RISCV instruction in hexadecimal format with --print-imm-hex flag.

jrtc27 · 2023-12-15T06:51:13Z

AArch64 uses decimal, as a data point, as do the various MIPS targets, PowerPC and x86, at least for a simple load with an offset.

To me this makes the output less user-friendly, and it's quite annoying churn for downstream. I've been perfectly happy with the output as it stands... Not to mention that it now deviates from what appears to be the standard practice.

So personally I'm extremely anti this patch.

topperc · 2023-12-15T07:10:29Z

AArch64 and X86 both use formatImm as far as I can see. How do they end up defaulting to decimal and RISC-V is defaulting to hex?

jrtc27 · 2023-12-15T07:16:51Z

Oh they've all been changed, that's awful. Is there an RFC about this tree-wide behavioural change that, IMO, is a regression?

jrtc27 · 2023-12-15T07:18:16Z

Note that this makes clang -S inconsistent with llvm-objdump -d; the former continues to use friendly decimal for things, whilst the latter uses hex, which isn't so user-friendly and feels rather against the spirit of disassembling things to look as close to human assembly as possible.

topperc · 2023-12-15T07:31:02Z

AArch64 made the change to use formatImm in 2016 http://reviews.llvm.org/D16929 Not sure what llvm-objdump defaulted to at the time.

topperc · 2023-12-15T07:33:43Z

Here's where objdump default changed https://reviews.llvm.org/D136972

MouseSplinter · 2024-01-02T09:40:09Z

Thanks for your guidance too much.

…imal format" (#75561) Reverts llvm/llvm-project#74053 Breaks https://lab.llvm.org/buildbot/#/builders/5/builds/39291 Co-authored-by: Wang Yaduo <wangyaduo@linux.alibaba.com> Issue #75563

llvmbot added backend:RISC-V mc Machine (object) code labels Dec 1, 2023

wangpc-pp requested review from asb, jrtc27, kito-cheng, topperc and wangpc-pp December 1, 2023 09:40

wangpc-pp approved these changes Dec 1, 2023

View reviewed changes

MouseSplinter force-pushed the riscv_objdum_imm_hex branch from bd8aa46 to 0e38197 Compare December 5, 2023 08:20

llvmbot added lld lld:ELF llvm:binary-utilities labels Dec 5, 2023

MouseSplinter force-pushed the riscv_objdum_imm_hex branch from 0e38197 to f008c9d Compare December 5, 2023 09:55

[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format

f008c9d

Enable the llvm-objdump to disassemble the immediate of RISCV instruction in hexadecimal format with --print-imm-hex flag.

topperc approved these changes Dec 6, 2023

View reviewed changes

compnerd approved these changes Dec 6, 2023

View reviewed changes

asb approved these changes Dec 6, 2023

View reviewed changes

MouseSplinter merged commit 3dde0d0 into llvm:main Dec 15, 2023
3 checks passed

MouseSplinter added a commit that referenced this pull request Dec 15, 2023

Revert "[RISCV] Support printing immediate of RISCV MCInst in hexadec…

689dcef

…imal format (#74053)" This reverts commit 3dde0d0.

vitalybuka mentioned this pull request Dec 15, 2023

Revert "[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format" #75561

Merged

vitalybuka pushed a commit that referenced this pull request Dec 15, 2023

[RISCV] Support printing immediate of RISCV MCInst in hexadecimal for…

c532ba4

…mat (#74053) Enable the llvm-objdump to disassemble the immediate of RISCV instruction in hexadecimal format with --print-imm-hex flag.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format #74053

[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format #74053

MouseSplinter commented Dec 1, 2023

llvmbot commented Dec 1, 2023 •

edited

wangpc-pp left a comment •

edited

MouseSplinter commented Dec 5, 2023

wangpc-pp commented Dec 5, 2023

topperc commented Dec 6, 2023

MouseSplinter commented Dec 6, 2023

topperc commented Dec 6, 2023

topperc left a comment

MouseSplinter commented Dec 6, 2023

asb left a comment

vitalybuka commented Dec 15, 2023

dyung commented Dec 15, 2023

MaskRay commented Dec 15, 2023 •

edited

jrtc27 commented Dec 15, 2023

topperc commented Dec 15, 2023

jrtc27 commented Dec 15, 2023

jrtc27 commented Dec 15, 2023

topperc commented Dec 15, 2023

topperc commented Dec 15, 2023

MouseSplinter commented Jan 2, 2024

[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format #74053

[RISCV] Support printing immediate of RISCV MCInst in hexadecimal format #74053

Conversation

MouseSplinter commented Dec 1, 2023

llvmbot commented Dec 1, 2023 • edited

wangpc-pp left a comment • edited

Choose a reason for hiding this comment

MouseSplinter commented Dec 5, 2023

wangpc-pp commented Dec 5, 2023

topperc commented Dec 6, 2023

MouseSplinter commented Dec 6, 2023

topperc commented Dec 6, 2023

topperc left a comment

Choose a reason for hiding this comment

MouseSplinter commented Dec 6, 2023

asb left a comment

Choose a reason for hiding this comment

vitalybuka commented Dec 15, 2023

dyung commented Dec 15, 2023

MaskRay commented Dec 15, 2023 • edited

jrtc27 commented Dec 15, 2023

topperc commented Dec 15, 2023

jrtc27 commented Dec 15, 2023

jrtc27 commented Dec 15, 2023

topperc commented Dec 15, 2023

topperc commented Dec 15, 2023

MouseSplinter commented Jan 2, 2024

llvmbot commented Dec 1, 2023 •

edited

wangpc-pp left a comment •

edited

MaskRay commented Dec 15, 2023 •

edited