Skip to content
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

[RISCV] Mark vmvNr.v as implicitly using vtype #118414

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Dec 2, 2024

This was pointed out in #118283 (comment). We cannot move these between vtype definitions as they depend on SEW and require vill to be clear.

This was pointed out in llvm#118283 (comment). We cannot move these between vtype definitions as they depend on SEW and require vill to be clear.
@llvmbot
Copy link
Member

llvmbot commented Dec 2, 2024

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

Author: Luke Lau (lukel97)

Changes

This was pointed out in #118283 (comment). We cannot move these between vtype definitions as they depend on SEW and require vill to be clear.


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

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfoV.td (+1-1)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
index 8e0c4826ac00de..6506b6746b1517 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
@@ -1726,7 +1726,7 @@ foreach n = [1, 2, 4, 8] in {
   def VMV#n#R_V  : RVInstV<0b100111, !add(n, -1), OPIVI, (outs vrc:$vd),
                            (ins vrc:$vs2), "vmv" # n # "r.v", "$vd, $vs2">,
                    VMVRSched<n> {
-    let Uses = [];
+    let Uses = [VTYPE];
     let vm = 1;
   }
 }

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@lukel97
Copy link
Contributor Author

lukel97 commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

@preames
Copy link
Collaborator

preames commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

LGTM as well, the mca change is at least reasonable if we're modeling a new runtime dependency which wasn't previously being modeled.

@topperc
Copy link
Collaborator

topperc commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

LGTM as well, the mca change is at least reasonable if we're modeling a new runtime dependency which wasn't previously being modeled.

Looking at the test. There are vsetvlis interleaved with vmv. So I assume llvm-mca is now waiting for the preceding vsetvli before it can start a vmv.

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