[RISCV] Fix More RV32 Signed Immediates #120658
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This extends the work done in 2b2b840 to cover all rv32 signed immediates. Binutils seems to accept these for branches and jumps, but doesn't for `c.addi16sp` - which I think is a bug on their side. How they are assembled by binutils looks a bit strange (with `32-bit branch; jump` pseudos and a relocation to an absolute symbol on the jump, even for the Zca instructions), but seems to do the equivalent thing.
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-risc-v Author: Sam Elliott (lenary) ChangesThis extends the work done in 2b2b840 to cover all rv32 signed immediates. Binutils seems to accept these for branches and jumps, but doesn't for Full diff: https://github.com/llvm/llvm-project/pull/120658.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 5b9946e5132e40..9dcf2e973e6c58 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -530,7 +530,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
if (!IsConstantImm)
IsValid = RISCVAsmParser::classifySymbolRef(getImm(), VK);
else
- IsValid = isShiftedInt<N - 1, 1>(Imm);
+ IsValid = isShiftedInt<N - 1, 1>(fixImmediateForRV32(Imm, isRV64Imm()));
return IsValid && VK == RISCVMCExpr::VK_RISCV_None;
}
@@ -943,7 +943,8 @@ struct RISCVOperand final : public MCParsedAsmOperand {
RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
int64_t Imm;
bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedInt<7, 5>(Imm) &&
+ return IsConstantImm &&
+ isShiftedInt<7, 5>(fixImmediateForRV32(Imm, isRV64Imm())) &&
VK == RISCVMCExpr::VK_RISCV_None;
}
@@ -955,7 +956,8 @@ struct RISCVOperand final : public MCParsedAsmOperand {
int64_t Imm;
RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && (Imm != 0) && isShiftedInt<6, 4>(Imm) &&
+ return IsConstantImm && (Imm != 0) &&
+ isShiftedInt<6, 4>(fixImmediateForRV32(Imm, isRV64Imm())) &&
VK == RISCVMCExpr::VK_RISCV_None;
}
diff --git a/llvm/test/MC/RISCV/rv32c-only-valid.s b/llvm/test/MC/RISCV/rv32c-only-valid.s
index 70f358ef24fce1..3321aff115c4d0 100644
--- a/llvm/test/MC/RISCV/rv32c-only-valid.s
+++ b/llvm/test/MC/RISCV/rv32c-only-valid.s
@@ -26,3 +26,31 @@ c.jal 2046
# CHECK-ASM: c.addi a1, -1
# CHECK-ASM: encoding: [0xfd,0x15]
c.addi a1, 0xffffffff
+
+# CHECK-OBJ: c.addi16sp sp, -352
+# CHECK-ASM: c.addi16sp sp, -352
+# CHECK-ASM: encoding: [0x0d,0x71]
+c.addi16sp sp, 0xfffffea0
+
+## Branch and Jump immediates are relative but printed as their absolute address
+## when disassembling.
+
+# CHECK-OBJ: c.beqz a2, 0xffffff06
+# CHECK-ASM: c.beqz a2, -256
+# CHECK-ASM: encoding: [0x01,0xd2]
+c.beqz a2, 0xffffff00
+
+# CHECK-OBJ: c.beqz a0, 0xffffff16
+# CHECK-ASM: .insn cb 1, 6, a0, -242
+# CHECK-ASM: encoding: [0x19,0xd5]
+.insn cb 1, 6, a0, 0xffffff0e
+
+# CHECK-OBJ: c.jal 0xfffffab4
+# CHECK-ASM: c.jal -1366
+# CHECK-ASM: encoding: [0x6d,0x34]
+c.jal 0xfffffaaa
+
+# CHECK-OBJ: c.j 0xfffffcd8
+# CHECK-ASM: .insn cj 1, 5, -820
+# CHECK-ASM: encoding: [0xf1,0xb1]
+.insn cj 1, 5, 0xfffffccc
diff --git a/llvm/test/MC/RISCV/rv32i-only-valid.s b/llvm/test/MC/RISCV/rv32i-only-valid.s
index 806219fd4b37d8..74232e3c419f14 100644
--- a/llvm/test/MC/RISCV/rv32i-only-valid.s
+++ b/llvm/test/MC/RISCV/rv32i-only-valid.s
@@ -2,7 +2,7 @@
# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
# RUN: llvm-mc -filetype=obj -triple=riscv32 < %s \
# RUN: | llvm-objdump -M no-aliases --no-print-imm-hex -d -r - \
-# RUN: | FileCheck -check-prefixes=CHECK-ASM-AND-OBJ %s
+# RUN: | FileCheck -check-prefixes=CHECK-OBJ,CHECK-ASM-AND-OBJ %s
# CHECK-ASM-AND-OBJ: addi a0, a1, -1
# CHECK-ASM: encoding: [0x13,0x85,0xf5,0xff]
@@ -16,3 +16,46 @@ lw a1, 0xfffff9ab(a2)
# CHECK-ASM-AND-OBJ: sw a1, -8(a2)
# CHECK-ASM: encoding: [0x23,0x2c,0xb6,0xfe]
sw a1, 0xfffffff8(a2)
+
+## Branch and Jump immediates are relative but printed as their absolute address
+## when disassembling.
+
+# CHECK-ASM: beq t0, t1, -4096
+# CHECK-ASM: encoding: [0x63,0x80,0x62,0x80]
+# CHECK-OBJ: beq t0, t1, 0xfffff010
+beq t0, t1, 0xfffff000
+
+# CHECK-ASM: bne t1, t2, -4082
+# CHECK-ASM: encoding: [0x63,0x17,0x73,0x80]
+# CHECK-OBJ: bne t1, t2, 0xfffff022
+bne t1, t2, 0xfffff00e
+
+# CHECK-ASM: beq t2, zero, -3550
+# CHECK-ASM: encoding: [0x63,0x81,0x03,0xa2]
+# CHECK-OBJ: beq t2, zero, 0xfffff23a
+beqz t2, 0xfffff222
+
+# CHECK-ASM: .insn b 99, 0, a0, a1, -3004
+# CHECK-ASM: encoding: [0x63,0x02,0xb5,0xc4]
+# CHECK-OBJ: beq a0, a1, 0xfffff460
+.insn b BRANCH, 0, a0, a1, 0xfffff444
+
+# CHECK-ASM: jal ra, -2458
+# CHECK-ASM: encoding: [0xef,0xf0,0x6f,0xe6]
+# CHECK-OBJ: jal ra, 0xfffff686
+jal ra, 0xfffff666
+
+# CHECK-ASM: jal ra, -1912
+# CHECK-ASM: encoding: [0xef,0xf0,0x9f,0x88]
+# CHECK-OBJ: jal ra, 0xfffff8ac
+jal 0xfffff888
+
+# CHECK-ASM: jal zero, -1366
+# CHECK-ASM: encoding: [0x6f,0xf0,0xbf,0xaa]
+# CHECK-OBJ: jal zero, 0xfffffad2
+j 0xfffffaaa
+
+# CHECK-ASM: .insn j 111, a0, -820
+# CHECK-ASM: encoding: [0x6f,0x65,0xe6,0xff]
+# CHECK-OBJ: jal a0, 0xfff6682a
+.insn j JAL, a0, 0xfffffccc
|
svs-quic
reviewed
Dec 20, 2024
lenary
changed the title
lenary
changed the title
lenary
commented
Dec 20, 2024
There was a problem hiding this comment.
This file has a strange name because rv32zcibop-valid.s contains both riscv32 and riscv64 RUN lines, which is annoying. I haven't checked how many others have the same property (or similarly for files starting rv64). It seems we have some rv<ext name>.s tests which might be better names for testcases covering both rv32 and rv64.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/6046 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/7852 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/4679 Here is the relevant piece of the build log for the reference |
qiaojbao
pushed a commit
to GPUOpen-Drivers/llvm-project
that referenced
this pull request
Feb 7, 2025
Local branch amd-gfx b0290dd Merged main:e138f7831e62 into amd-gfx:4f96d7935439 Remote branch main c361fd5 [RISCV] Fix More RV32 Signed Immediates (llvm#120658)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This extends the work done in 2b2b840 to cover all rv32 signed immediates.
Binutils seems to accept these for branches and jumps, but doesn't for
c.addi16sp- which I think is a bug on their side. How they are assembled by binutils looks a bit strange (with32-bit branch; jumppseudos and a relocation to an absolute symbol on the jump, even for the Zca instructions), but seems to do the equivalent thing.