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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM



================
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)
----------------
kbarton wrote:
> 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. 
> 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?

I'm talking about any spill/reload code at all. I think this is okay, because...

isCallerPreservedPhysReg is defined as follows:

  /// Physical registers that may be modified within a function but are
  /// guaranteed to be restored before any uses. This is useful for targets that
  /// have call sequences where a GOT register may be updated by the caller
  /// prior to a call and is guaranteed to be restored (also by the caller)
  /// after the call. 
  virtual bool isCallerPreservedPhysReg(unsigned PhysReg,
                                        const MachineFunction &MF) const {
    return false;
  }

So what we know about such a register is that it is the same at any use. However, there may be defs of the register that are required in order to maintain that invariant. The defs are going to be loads, for example, and if you ignore the fact that these define a physical register, then what prevents them from being CSE'd? So we can't do that. However, the change here is only for uses, not defs, so I think we're okay. I think, however, you specifically can't make the corresponding change in the loop below for defs.


================
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)
----------------
hfinkel wrote:
> kbarton wrote:
> > 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. 
> > 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?
> 
> I'm talking about any spill/reload code at all. I think this is okay, because...
> 
> isCallerPreservedPhysReg is defined as follows:
> 
>   /// Physical registers that may be modified within a function but are
>   /// guaranteed to be restored before any uses. This is useful for targets that
>   /// have call sequences where a GOT register may be updated by the caller
>   /// prior to a call and is guaranteed to be restored (also by the caller)
>   /// after the call. 
>   virtual bool isCallerPreservedPhysReg(unsigned PhysReg,
>                                         const MachineFunction &MF) const {
>     return false;
>   }
> 
> So what we know about such a register is that it is the same at any use. However, there may be defs of the register that are required in order to maintain that invariant. The defs are going to be loads, for example, and if you ignore the fact that these define a physical register, then what prevents them from being CSE'd? So we can't do that. However, the change here is only for uses, not defs, so I think we're okay. I think, however, you specifically can't make the corresponding change in the loop below for defs.
> If we put this into a separate check, which then contains the checks for isConstantPhysReg and isCallerPreservedPhysReg it might be more clear.

We could make a utility member function contains both calls. Something like:

  bool isCallerPreservedOrConstPhysReg(unsigned PhysReg, const MachineFunction &MF) const {
    const MachineRegisterInfo &MRI = MF.getRegInfo();
    return MRI.isConstantPhysReg(PhysReg) || isCallerPreservedPhysReg(PhysReg, MF);
}

This seems reasonable.



https://reviews.llvm.org/D39173