[PATCH] D15667: [MachineScheduler] Handle regmasks and allow calls to be rescheduled.
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 27 13:21:13 PDT 2016
MatzeB added inline comments.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:324-329
@@ +323,8 @@
+ Reg2SUnitsMap::iterator I = P.second;
+ for (bool isBegin = I == B; !isBegin; /* empty */) {
+ isBegin = (--I) == B;
+ if (I->SU->isCall)
+ I = Defs.erase(I);
+ }
+}
+
----------------
+1
Also arguments to calls are not necessarily dead, they may reside in callee saved registers. Simply forgetting about all call-induced Defs seems invalid to me.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:334-336
@@ +333,5 @@
+
+ // Make sure we don't move reserved register definitions across
+ // calls. (This would be unnecessary if they were guaranteed to
+ // always be part of a regmask operand on each call. Are they?)
+ for (unsigned reg = 1; reg < TRI->getNumRegs(); reg++) {
----------------
Have you actually tried without this code. If the call potentially reads/writes a reserved register then it must announce that with a use/def machine operand. If it does not this is a bug that should be fixed in the target IMO.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:360-361
@@ +359,4 @@
+
+/// addRegMaskDeps - Handle regmasks to be able to reschedule around
+/// calls.
+void ScheduleDAGInstrs::addRegMaskDeps(SUnit *SU, unsigned OperIdx) {
----------------
Don't repeat the function name in the doxygen comment.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:366
@@ +365,3 @@
+
+ for (unsigned reg = 1; reg < TRI->getNumRegs(); reg++) {
+ if (MO.clobbersPhysReg(reg)) {
----------------
Variable names should start with uppercase letter.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:367
@@ +366,3 @@
+ for (unsigned reg = 1; reg < TRI->getNumRegs(); reg++) {
+ if (MO.clobbersPhysReg(reg)) {
+ // Add output depencencies on all clobberd registers. Calls are
----------------
Avoid indentation with `if (!MO.clobbersPhysReg(reg)) continue;`
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:371
@@ +370,3 @@
+ // they are not handled here.
+ for (MCRegAliasIterator Alias(reg, TRI, true); Alias.isValid(); ++Alias) {
+ for (Reg2SUnitsMap::iterator I = Defs.find(*Alias); I != Defs.end(); ++I) {
----------------
register masks already contain all register, it should not be necessary to iterate over the aliases.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:378
@@ +377,3 @@
+ // Don't add dependency to another dead def or another regmask.
+ bool defOp = DefSU->getInstr()->definesRegister(*Alias);
+ if (DefSU != SU && defOp && !SURegDefIsDead(DefSU, *Alias)) {
----------------
Variable name should start with upper case letter. This call could be deferred until we know that `DefSU != SU`.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:379
@@ +378,3 @@
+ bool defOp = DefSU->getInstr()->definesRegister(*Alias);
+ if (DefSU != SU && defOp && !SURegDefIsDead(DefSU, *Alias)) {
+ SDep Dep(SU, SDep::Output, /*Reg=*/*Alias);
----------------
Isn't `defOp` equivalent to `!SURegDefIsDead()`?
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:380
@@ +379,3 @@
+ if (DefSU != SU && defOp && !SURegDefIsDead(DefSU, *Alias)) {
+ SDep Dep(SU, SDep::Output, /*Reg=*/*Alias);
+ DefSU->addPred(Dep);
----------------
Maybe introduce a variable `unsigned AliasReg = *Alias`; so we don't need to remind ourself with a comment that `*Alias` is a register...
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:386-387
@@ +385,4 @@
+
+ if (SU->isCall)
+ clearCallsInDefsForReg(reg);
+ Defs.insert(PhysRegSUOper(SU, -1, reg));
----------------
see above, I would expect it to simply erase all previous Defs for this register `reg`.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:609-610
@@ -544,1 +608,4 @@
+/// Return true if SU has a dead register def operand of Reg, or a
+/// regmask that clobbers it, without having a live def of it as well.
+bool ScheduleDAGInstrs::SURegDefIsDead(const SUnit *SU, unsigned Reg) {
----------------
`\p SU` and `\p Reg` is better doxygen style.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:611
@@ +610,3 @@
+/// regmask that clobbers it, without having a live def of it as well.
+bool ScheduleDAGInstrs::SURegDefIsDead(const SUnit *SU, unsigned Reg) {
+ assert (TRI->isPhysicalRegister(Reg));
----------------
I'd tend to pass a `const MachineInstr &` instead of `const SUnit*` here (nullptr is invalid and MachineInstr instead of SUnit makes the function useful in more contexts).
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:612
@@ +611,3 @@
+bool ScheduleDAGInstrs::SURegDefIsDead(const SUnit *SU, unsigned Reg) {
+ assert (TRI->isPhysicalRegister(Reg));
+ bool hasDeadDef = false;
----------------
No space before `(`.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:613
@@ +612,3 @@
+ assert (TRI->isPhysicalRegister(Reg));
+ bool hasDeadDef = false;
+ MachineInstr *MI = SU->getInstr();
----------------
upper case.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:615
@@ +614,3 @@
+ MachineInstr *MI = SU->getInstr();
+ for (const auto &I : MI->operands()) {
+ if (I.isRegMask() && I.clobbersPhysReg(Reg))
----------------
Please avoid `auto` in cases where the type is not obvious, we also tend to use the variable name `MO` for machine operands in most other loops.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:618-623
@@ +617,8 @@
+ hasDeadDef = true;
+ else if (I.isReg() && I.isDef() && I.getReg() == Reg) {
+ if (I.isDead())
+ hasDeadDef = true;
+ else
+ return false;
+ }
+ }
----------------
Do we need to take register aliases into account here?
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:626
@@ +625,3 @@
+
+ return (hasDeadDef);
+}
----------------
no braces.
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:1035-1036
@@ -950,2 +1034,4 @@
const MachineOperand &MO = MI->getOperand(j);
+ if (MO.isRegMask())
+ addRegMaskDeps(SU, j);
if (!MO.isReg()) continue;
----------------
You can add a `continue;` here
================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:1053-1054
@@ +1052,4 @@
+
+ if (SU->isCall)
+ addCallDeps(SU);
+
----------------
See my comment in addCallDeps(). I really think calls should not be special in any way regarding register dependencies...
http://reviews.llvm.org/D15667
More information about the llvm-commits
mailing list