Skip to content

Conversation

@michalt
Copy link
Contributor

@michalt michalt commented Aug 29, 2024

Currently we multiply the default (SiFive7GetCyclesDefault) by 10, but this turns out to be both surprising to our users and leads to worse codegen in most cases. I think it's more natural to just keep the default.

In the end the right solution is probably to have a separate scheduling model for a particular VCIX coprocessor.

Currently we multiply the default (`SiFive7GetCyclesDefault`) by 10, but
this turns out to be both surprising to our users and leads to worse
codegen in most cases. I think it's more natural to just keep the
default.

In the end the right solution is probably to have a separate scheduling
model for a particular VCIX coprocessor.
@llvmbot
Copy link
Member

llvmbot commented Aug 29, 2024

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

Author: Michal Terepeta (michalt)

Changes

Currently we multiply the default (SiFive7GetCyclesDefault) by 10, but this turns out to be both surprising to our users and leads to worse codegen in most cases. I think it's more natural to just keep the default.

In the end the right solution is probably to have a separate scheduling model for a particular VCIX coprocessor.


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

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVSchedSiFive7.td (+1-1)
diff --git a/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td b/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
index 0b0ac0c368d070..72560284dde2f5 100644
--- a/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
+++ b/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
@@ -973,7 +973,7 @@ def : InstRW<[WriteIALU], (instrs COPY)>;
 foreach mx = SchedMxList in {
   defvar Cycles = SiFive7GetCyclesDefault<mx>.c;
   defvar IsWorstCase = SiFive7IsWorstCaseMX<mx, SchedMxList>.c;
-  let Latency = !mul(Cycles, 10),
+  let Latency = Cycles,
       AcquireAtCycles = [0, 1],
       ReleaseAtCycles = [1, !add(1, Cycles)] in {
     defm "" : LMULWriteResMX<"WriteVC_V_I",   [SiFive7VCQ, SiFive7VA], mx, IsWorstCase>;

@michalt
Copy link
Contributor Author

michalt commented Aug 29, 2024

@topperc @michaelmaitland Would you be ok with this change?

Copy link
Contributor

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

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

I think this is reasonable. Left a small nit.

Copy link
Contributor

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

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

LGTM

@michalt
Copy link
Contributor Author

michalt commented Sep 4, 2024

@michaelmaitland Thank you for the review! 😀 Could I also ask you to merge? (I don't have commit access)

@michaelmaitland michaelmaitland merged commit 54194e1 into llvm:main Sep 5, 2024
@michalt
Copy link
Contributor Author

michalt commented Sep 6, 2024

Thanks! 😄

@michalt michalt deleted the mul10 branch September 6, 2024 06:50
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