Page MenuHomePhabricator

[MachineCSE] Add new callback for is caller preserved or constant physregs.
ClosedPublic

Authored by jtony on Oct 22 2017, 6:30 PM.

Details

Summary

The instructions addis,addi, bl are used to calculate the address of TLS thread local variables. These TLS access code sequences are generated repeatedly every time the thread local variable is accessed. By communicating to Machine CSE that X2 is guaranteed to have the same value within the same function call (so called Caller Preserved Physical Register), the redundant TLS access code sequences are cleaned up.

Note: for the test case included in the patch, before the fix the same TLS access code sequences (addis/addi/bl) is generated twice. After the fix, it is only generated once. The test case tests the new behaviour after the fix.

Diff Detail

Repository
rL LLVM

Event Timeline

jtony created this revision.Oct 22 2017, 6:30 PM
kbarton added inline comments.Oct 23 2017, 6:45 AM
lib/CodeGen/MachineCSE.cpp
255 ↗(On Diff #119806)

Is it not possible to put this check into isConstantPhysReg instead?

hfinkel added inline comments.Oct 23 2017, 12:48 PM
lib/CodeGen/MachineCSE.cpp
255 ↗(On Diff #119806)

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?

jtony updated this revision to Diff 119933.Oct 23 2017, 2:07 PM
jtony marked 2 inline comments as done.
jtony added inline comments.
lib/CodeGen/MachineCSE.cpp
255 ↗(On Diff #119806)

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.

kbarton added inline comments.Oct 24 2017, 6:51 AM
lib/CodeGen/MachineCSE.cpp
255 ↗(On Diff #119806)

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.

hfinkel accepted this revision.Oct 26 2017, 8:24 PM

LGTM

lib/CodeGen/MachineCSE.cpp
255 ↗(On Diff #119806)

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.

255 ↗(On Diff #119806)

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.

This revision is now accepted and ready to land.Oct 26 2017, 8:24 PM

Oh, and please retitle this before you commit. You should talk about caller-preserved registers, not TLS.

jtony retitled this revision from [MachineCSE] Remove redudant TLS access instructions. to [MachineCSE] Add new callback for is caller preserved or constant physregs..Nov 16 2017, 10:17 AM
This revision was automatically updated to reflect the committed changes.
jtony marked an inline comment as done.