Skip to content

[RISCV] Update comment on -w stripping pass. NFC #67415

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 1 commit into from
Sep 26, 2023

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Sep 26, 2023

It looks like we only strip the -w suffix from addw and not addiw (because c.addiw and c.addi have the same register encoding), but the comment in the header seems to have it the other way round.

It looks like we only strip the -w suffix from addw instructions, and not addiw
instructions (bedcause c.addiw and c.addi have the same register encoding), but
the comment in the header seemed to have it the other way round.
@llvmbot
Copy link
Member

llvmbot commented Sep 26, 2023

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

Changes

It looks like we only strip the -w suffix from addw and not addiw (bedcause c.addiw and c.addi have the same register encoding), but the comment in the header seems to have it the other way round.


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

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp (+4-3)
diff --git a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
index bd294c669735f4f..3c608bf8b50b57d 100644
--- a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
+++ b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
@@ -12,10 +12,11 @@
 // extended bits aren't consumed or because the input was already sign extended
 // by an earlier instruction.
 //
-// Then it removes the -w suffix from each addiw and slliw instructions
+// Then it removes the -w suffix from addw, slliw and mulw instructions
 // whenever all users are dependent only on the lower word of the result of the
-// instruction. We do this only for addiw, slliw, and mulw because the -w forms
-// are less compressible.
+// instruction. We do this only for addw, slliw, and mulw because the -w forms
+// are less compressible: c.add and c.slli have a larger register encoding than
+// their w counterparts, and there's no compressible version of mulw.
 //
 //===---------------------------------------------------------------------===//
 

@preames
Copy link
Collaborator

preames commented Sep 26, 2023

A bit off topic, but should we be restricting this only to the current list? I've been noticing a bunch of tests recently with RV32 vs RV64 differences on ADDIW vs ADDI where the difference is purely syntactic. Maybe from a testing perspective alone, we should just strip all the legal w suffixes as long as it doesn't decrease compressability?

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 lukel97 merged commit 733e3c6 into llvm:main Sep 26, 2023
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
It looks like we only strip the -w suffix from addw and not addiw
(because c.addiw and c.addi have the same register encoding), but the
comment in the header seems to have it the other way round.
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