[PATCH] D39173: [MachineCSE] Remove redudant TLS access instructions.

[Kit Barton via Phabricator via llvm-commits]

kbarton added inline comments.


================
Comment at: lib/CodeGen/MachineCSE.cpp:255
+    if (!MRI->isConstantPhysReg(Reg) &&
+        !(TRI->isCallerPreservedPhysReg(Reg, *MI->getParent()->getParent())))
       for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
----------------
jtony wrote:
> hfinkel wrote:
> > kbarton wrote:
> > > Is it not possible to put this check into isConstantPhysReg instead? 
> > > 
> > No, I don't think so. We don't track register dependencies at all (e.g., during instruction scheduling) for constant phys regs. We can't add r2 to that constant set (because we need scheduling to understand the constraints on the r2 save/restore code).
> > 
> > isCallerPreservedPhysReg can only be used if we know that we're not dealing with transformations that can disturb the save/restore sequences. Is this generally safe here?
> Thanks Hal for your quick reply.  I agree it is not safe to put the isCallerPreservedPhysReg check into isConstantPhysReg.  About your question, are you talking about any spill/reload code at all or you are just talking about prologue/epilogue? If you are talking about the prologue/epilogue, that (PEI) happens after the MCSE, so MCSE cannot disturb the save/restore sequences there (in PEI)  it should be safe to do so.
Yes, that's a good point Hal.

Would it be better to then change this into a separate check that could encapsulate both of these concepts (and maybe others) into a single check? In this case, what we really want is to ensure the contents of Reg are not going to change throughout the body of the function, which is slightly different than what isConstantPhysReg provides. If we put this into a separate check, which then contains the checks for isConstantPhysReg and isCallerPreservedPhysReg it might be more clear.

My thinking is that I suspect there are other places where this same check would be useful. By wrapping this into a separate callback it makes it easier to maintain. The downside being the introduction of yet another callback in the backends. 


https://reviews.llvm.org/D39173