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] Don't call use_nodbg_operands for physical registers in RISCVOptWInstrs hasAllNBitUsers. #77032

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jan 5, 2024

The ADDIW in the new test case was incorrectly removed due to incorrectly following the x10 register from the return value back to the argument. This is due to use_nodbg_operands returning every instruction that uses a physical register regardless of the data flow.

…OptWInstrs hasAllNBitUsers.

The ADDIW in the new test case was incorrectly removed due to
incorrectly following the x10 register from the return value back
to the argument.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 5, 2024

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

Author: Craig Topper (topperc)

Changes

The ADDIW in the new test case was incorrectly removed due to incorrectly following the x10 register from the return value back to the argument. This is due to use_nodbg_operands returning every instruction that uses a physical register regardless of the data flow.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp (+3)
  • (modified) llvm/test/CodeGen/RISCV/opt-w-instrs.mir (+20)
diff --git a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
index 2c2b34bb5b7797..b9dde29d05a369 100644
--- a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
+++ b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
@@ -126,6 +126,9 @@ static bool hasAllNBitUsers(const MachineInstr &OrigMI,
     if (MI->getNumExplicitDefs() != 1)
       return false;
 
+    if (!MI->getOperand(0).getReg().isVirtual())
+      return false;
+
     for (auto &UserOp : MRI.use_nodbg_operands(MI->getOperand(0).getReg())) {
       const MachineInstr *UserMI = UserOp.getParent();
       unsigned OpIdx = UserOp.getOperandNo();
diff --git a/llvm/test/CodeGen/RISCV/opt-w-instrs.mir b/llvm/test/CodeGen/RISCV/opt-w-instrs.mir
index 0ecf8fd6bef33a..ebac5a42fbcda0 100644
--- a/llvm/test/CodeGen/RISCV/opt-w-instrs.mir
+++ b/llvm/test/CodeGen/RISCV/opt-w-instrs.mir
@@ -28,3 +28,23 @@ body:             |
     $x11 = COPY %4
     PseudoRET
 ...
+
+---
+name:            physreg
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    liveins: $x10, $x11
+
+    ; CHECK-ZFA-LABEL: name: physreg
+    ; CHECK-ZFA: liveins: $x10, $x11
+    ; CHECK-ZFA-NEXT: {{  $}}
+    ; CHECK-ZFA-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
+    ; CHECK-ZFA-NEXT: [[ADDIW:%[0-9]+]]:gpr = ADDIW [[COPY]], 0
+    ; CHECK-ZFA-NEXT: $x10 = COPY [[ADDIW]]
+    ; CHECK-ZFA-NEXT: PseudoRET
+    %0:gpr = COPY $x10
+    %1:gpr = ADDIW %0, 0
+    $x10 = COPY %1
+    PseudoRET
+...

Comment on lines 129 to 132
if (!MI->getOperand(0).getReg().isVirtual())
return false;

for (auto &UserOp : MRI.use_nodbg_operands(MI->getOperand(0).getReg())) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we factor out MI->getOperand(0).getReg() into its own variable?

Copy link
Member

@mshockwave mshockwave left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@topperc topperc merged commit 4dd5d96 into llvm:main Jan 5, 2024
4 checks passed
@topperc topperc deleted the pr/opt-w-physreg branch January 5, 2024 17:22
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.

3 participants