-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[MachineScheduler] Fix physreg dependencies of ExitSU #123541
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
Changes from 2 commits
05cca1b
8b87e2f
d709f4b
806fa90
b7cfe8b
e51732a
38cf1e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -209,13 +209,25 @@ void ScheduleDAGInstrs::addSchedBarrierDeps() { | |||
| ExitSU.setInstr(ExitMI); | ||||
| // Add dependencies on the defs and uses of the instruction. | ||||
| if (ExitMI) { | ||||
| const MCInstrDesc &MIDesc = ExitMI->getDesc(); | ||||
| for (const MachineOperand &MO : ExitMI->all_uses()) { | ||||
| unsigned OpIdx = MO.getOperandNo(); | ||||
| Register Reg = MO.getReg(); | ||||
| if (Reg.isPhysical()) { | ||||
| // addPhysRegDataDeps uses the provided operand index to retrieve | ||||
| // the operand use cycle from the scheduling model. If the operand | ||||
| // is "fake" (e.g., an operand of a call instruction used to pass | ||||
| // an argument to the called function.), the scheduling model may not | ||||
| // have an entry for it. If this is the case, pass -1 as operand index, | ||||
| // which will cause addPhysRegDataDeps to add an artificial dependency. | ||||
| // FIXME: Using hasImplicitUseOfPhysReg here is inaccurate as it misses | ||||
| // aliases. When fixing, make sure to update addPhysRegDataDeps, too. | ||||
| bool IsRealUse = OpIdx < MIDesc.getNumOperands() || | ||||
| MIDesc.hasImplicitUseOfPhysReg(Reg); | ||||
| for (MCRegUnit Unit : TRI->regunits(Reg)) | ||||
| Uses.insert(PhysRegSUOper(&ExitSU, -1, Unit)); | ||||
| Uses.insert(PhysRegSUOper(&ExitSU, IsRealUse ? OpIdx : -1, Unit)); | ||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where an operand index gets a negative value. There is one more place at the end of this function. |
||||
| } else if (Reg.isVirtual() && MO.readsReg()) { | ||||
| addVRegUseDeps(&ExitSU, MO.getOperandNo()); | ||||
| addVRegUseDeps(&ExitSU, OpIdx); | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
@@ -263,6 +275,9 @@ void ScheduleDAGInstrs::addPhysRegDataDeps(SUnit *SU, unsigned OperIdx) { | |||
| bool ImplicitPseudoUse = false; | ||||
| SDep Dep; | ||||
| if (UseOpIdx < 0) { | ||||
| // FIXME: UseOpIdx can be passed to computeOperandLatency, which can | ||||
| // pass it to findUseIdx, which treats it as unsigned. If this is | ||||
| // the expected behavior, it should be commented. | ||||
|
||||
| if (UseOpIdx < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to improve this comment, please see if it makes more sense now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should crash if
UseOperIdx == -1, except it doesn't. This function is only called (under certain conditions) if the target uses SchedModel (not Itineraries). Probably this code is unreachable for in-tree models.
I did some debugging. It appears that when -1 is passed to computeOperandLatency, UseInstr is always nullptr. Operand index is only used if UseInstr is non-null, so strictly speaking there is no bug. I'm going to drop this comment.
(Let me know if I should do something different, like add a note or assertion somewhere.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment this usage. I'm assuming getting this accurate isn't required for correctness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIDesc.hasImplicitUseOfPhysReg also looks buggy to me. It doesn't have the MCRI argument like hasImplicitDefOfPhysReg, so it will miss any aliases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I'm aware, getting this accurate isn't required for correctness. My motivation for fixing this was to move comparisons away from branches if the latency between them is high (which is true on my target).
Good point, I'll add a FIXME.