Skip to content

[RISCV][WIP] Add validation of SPIMM for cm.push/pop. #84989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Mar 12, 2024

This checks the immediate is aligned which would catch what #84935 is fixing

Obviously this will fail tests until the issues are fixed.

This checks the immediate is aligned which would catch what

Obviously this will fail until the issues are fixed.
@llvmbot
Copy link
Member

llvmbot commented Mar 12, 2024

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

Author: Craig Topper (topperc)

Changes

This checks the immediate is aligned which would catch what #83457 is fixing

Obviously this will fail tests until the issues are fixed.


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

3 Files Affected:

  • (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h (+2-1)
  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+3)
  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfoZc.td (+2-1)
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
index 6d0381c30d3e86..80f6ad9bb8aefe 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
@@ -297,7 +297,8 @@ enum OperandType : unsigned {
   OPERAND_RVKRNUM_0_7,
   OPERAND_RVKRNUM_1_10,
   OPERAND_RVKRNUM_2_14,
-  OPERAND_LAST_RISCV_IMM = OPERAND_RVKRNUM_2_14,
+  OPERAND_SPIMM,
+  OPERAND_LAST_RISCV_IMM = OPERAND_SPIMM,
   // Operand is either a register or uimm5, this is used by V extension pseudo
   // instructions to represent a value that be passed as AVL to either vsetvli
   // or vsetivli.
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 37a8079dcbf10d..15453a5c5e5924 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -2050,6 +2050,9 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
         case RISCVOp::OPERAND_RVKRNUM_2_14:
           Ok = Imm >= 2 && Imm <= 14;
           break;
+        case RISCVOp::OPERAND_SPIMM:
+          Ok = (Imm & 0xf) == 0;
+          break;
         }
         if (!Ok) {
           ErrInfo = "Invalid immediate";
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
index 2c8451c5c4ceb2..b66c28f9fd54ef 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
@@ -61,10 +61,11 @@ def rlist : Operand<OtherVT> {
   }];
  }
 
-def spimm : Operand<OtherVT> {
+def spimm : RISCVOp<OtherVT> {
   let ParserMatchClass = SpimmAsmOperand;
   let PrintMethod = "printSpimm";
   let DecoderMethod = "decodeZcmpSpimm";
+  let OperandType = "OPERAND_SPIMM";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (!MCOp.evaluateAsConstantImm(Imm))

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
(I think you copied the wrong PR ID :-))

@nemanjai
Copy link
Member

LGTM. Just an idea - perhaps we should have a test or two with non-multiples of 16 in test/MC/RISCV/rv{64|32}zcmp-invalid.s

@topperc
Copy link
Collaborator Author

topperc commented Mar 13, 2024

LGTM. (I think you copied the wrong PR ID :-))

I definitely did. Fixed

topperc added a commit to topperc/llvm-project that referenced this pull request Mar 21, 2024
…h/pop.

This an alternative to llvm#84935 to fix the miscompile, but not be
optimal.

The immediate for cm.push/pop must be a multiple of 16. For RVE,
it might not be. It's not easy to increase the stack size without
messing up cfa directives and maybe other things.

This patch rounds the stack size down to a multiple of 16 before
clamping it to 48. This causes an extra addi to be emitted to handle the
remainder.

Once this commited, I can commit llvm#84989 to add verification for these
instructions being generated with valid offsets.
topperc added a commit that referenced this pull request Mar 27, 2024
…h/pop. (#86073)

This an alternative to #84935 to fix the miscompile, but not be optimal.

The immediate for cm.push/pop must be a multiple of 16. For RVE, it
might not be. It's not easy to increase the stack size without messing
up cfa directives and maybe other things.

This patch rounds the stack size down to a multiple of 16 before
clamping it to 48. This causes an extra addi to be emitted to handle the
remainder.

Once this commited, I can commit #84989 to add verification for these
instructions being generated with valid offsets.
@topperc topperc merged commit 152fcf6 into llvm:main Mar 28, 2024
@topperc topperc deleted the pr/verify-spimm branch March 28, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants