Skip to content

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Apr 2, 2024

It makes no sense that shift operation uses the I0/I1 anyway as opposed to M, especially when the more complex instruction uses the simpler pipeline. We should assume both use M pipeline, and a note should be made that this is a deviation from the doc because of its high likelihood of being an error.

@llvmbot
Copy link
Member

llvmbot commented Apr 2, 2024

@llvm/pr-subscribers-backend-arm

Author: AtariDreams (AtariDreams)

Changes

It makes no sense that shift operation uses the I0/I1 anyway as opposed to M, especially when the more complex instruction uses the simpler pipeline. We should assume both use M pipeline, and a note should be made that this is a deviation from the doc because of its high likelihood of being an error.


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

1 Files Affected:

  • (modified) llvm/lib/Target/ARM/ARMScheduleA57.td (+7-3)
diff --git a/llvm/lib/Target/ARM/ARMScheduleA57.td b/llvm/lib/Target/ARM/ARMScheduleA57.td
index 3baac6b233c458..cb78752ecdaf15 100644
--- a/llvm/lib/Target/ARM/ARMScheduleA57.td
+++ b/llvm/lib/Target/ARM/ARMScheduleA57.td
@@ -181,13 +181,17 @@ class A57BranchForm<SchedWriteRes non_br> :
 // shift by register, conditional or unconditional
 // TODO: according to the doc, conditional uses I0/I1, unconditional uses M
 // Why more complex instruction uses more simple pipeline?
-// May be an error in doc.
+
+// It makes no sense that a shift operation uses the I0/I1 anyway as opposed to M,
+and this is the only operation to do so, so it makes logical sense that both
+// actually use the M pipeline.
+
 def A57WriteALUsr : SchedWriteVariant<[
-  SchedVar<IsPredicatedPred, [CheckBranchForm<0, A57BranchForm<A57Write_2cyc_1I>>]>,
+  SchedVar<IsPredicatedPred, [CheckBranchForm<0, A57BranchForm<A57Write_2cyc_1M>>]>,
   SchedVar<NoSchedPred,      [CheckBranchForm<0, A57BranchForm<A57Write_2cyc_1M>>]>
 ]>;
 def A57WriteALUSsr : SchedWriteVariant<[
-  SchedVar<IsPredicatedPred, [CheckBranchForm<0, A57BranchForm<A57Write_2cyc_1I>>]>,
+  SchedVar<IsPredicatedPred, [CheckBranchForm<0, A57BranchForm<A57Write_2cyc_1M>>]>,
   SchedVar<NoSchedPred,      [CheckBranchForm<0, A57BranchForm<A57Write_2cyc_1M>>]>
 ]>;
 def A57ReadALUsr : SchedReadVariant<[

@AZero13 AZero13 force-pushed the switch branch 2 times, most recently from 66af3c1 to d7c7dd0 Compare April 2, 2024 15:13
It makes no sense that shift operation uses the I0/I1 anyway
as opposed to M, especially when the more complex instruction
uses the simpler pipeline. We should assume both use M pipeline,
and a note should be made that this is a deviation from the doc
because of its high likelihood of being an error.
@DavidSpickett
Copy link
Collaborator

@davemgreen maybe?

@AZero13 AZero13 marked this pull request as draft April 2, 2024 16:52
@AZero13 AZero13 closed this Jun 24, 2024
@AZero13 AZero13 deleted the switch branch June 24, 2024 00:28
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