Expand subregisters in MachineFrameInfo::getPristineRegs

[kparzysz]

Summary

Solve the previously described problem by adding subregisters of pristine registers to the bit vector returned by getPristineRegs.

[qcolombet]

Hi Krzysztof,

Unfortunately, I think this is not correct to use the RegUnit for now.
Matthias may remember the details, but right now, we have some weird situation where for instance XMM0 is pristine, but YMM0 is not, whereas they are actually the same regunit.

So, for the target, that means that it has to list all the registers that are actually preserved.

Note, my recollection may be wrong and in that case just ignore my comment, but please double check!

Thanks,
-Quentin

[kparzysz]

How do you prevent the scavenger from clobbering YMM0 then? Do you think that a similar problem could also happen on X86 in some circumstances?

[kparzysz]

Listing both, 32-bit registers and their 64-bit super-registers as callee-saved does not really solve the problem. For example, if callee-saved = { R16, R17, D8 }, and actually-saved = { D8 }, then there are no pristine registers since R16 and R17 are both covered by D8. The scavenger will still calculate the set-difference and come up with { R16, R17 }, and consider those to be pristine. The "actually-saved" information is used in PEI to generate the save/restore instructions. If that set also contained sub-registers, we would have to add checks to our Hexagon-specific hook to avoid duplicate loads/stores. I'm not sure if all callers of "getCalleeSavedInfo" would be aware of such potential redundancy.

Edit: defeating remarkup failed

[MatzeB]

• Next time, please upload the patch with full context (see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)
• It is technically possible to track pristine registers as register units, if you track things at the regunit level you are more conservative with respect to the XMM0/YMM0 problem but that shouldn't be a problem in this case. But:

I doesn't look like this change actually simplifies the code: While RegisterScavenger.cpp gets simpler with this patch, AggressiveAntiDepBreaker.cpp, CriticalAntiDepBreaker.cpp, LivePhysRegs.cpp and MachineVerifier.cpp get more complicated.

I'd rather change getPristineRegs() to something like this to fix the bug:

BitVector MachineFrameInfo::getPristineRegs(const MachineFunction &MF) const {
  const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();
  BitVector BV(TRI->getNumRegs());

  // Before CSI is calculated, no registers are considered pristine. They can be
  // freely used and PEI will make sure they are saved.
  if (!isCalleeSavedInfoValid())
    return BV;

  for (const MCPhysReg *CSR = TRI->getCalleeSavedRegs(&MF); CSR && *CSR; ++CSR)
    BV.set(*CSR);

  // Saved registers and their subregisters are not pristine.
  const std::vector<CalleeSavedInfo> &CSI = getCalleeSavedInfo();
  for (const CalleeSavedInfo &Info : CSI) {
    for (MCSubRegIterator R(Info.getReg(), TRI, true); R.isValid(); ++R)
      BV.reset(*R);
  }

  return BV;
}

would that work for you?

[kparzysz]

I know that the code does not become simpler, I just wanted to use a common measure of register use. What you are proposing would work for our case, I'd only add the traversal of subregisters to the CSR loop as well (to cover the opposite case).

There is also one other place where the pristine registers are calculated: addPristines in LivePhysReg.cpp. Should that function call MachineFrameInfo::getPristineRegs?

[qcolombet]

For the record, I am kind of uncomfortable with Matthias’ suggestion because we slightly modify the output of the function. This is not wrong of course, but I believe that for most backend this is not needed because they already know what the output was and were doing the right thing. I.e., we add a compile time overhead for no benefit.

My concern is that this may have a significant impact on backend with a lot of registers (i.e., GPUs). I believe the effect would be mitigated by the fact that, those usually don’t have calls, i.e., they likely don’t have CSRs saved.

The bottom line is if we go with that change we should be aware of potentiel downside for everybody and we need to balance whether a target specific fix would be more appropriate.

That being said, Matthias seems fine with the suggested change, so we might just carry on.

Cheers,
-Quentin

[kparzysz]

In D14719#291028, @qcolombet wrote:

[...] whether a target specific fix would be more appropriate.

What type of approach do you have in mind for that? Targets that call this function themselves will sure know what to expect, my concern is about target-independent code. It doesn't seem like that the function getPristineRegs has a precisely defined result: it seems that the current assumption is that it will be a set of registers that is a subset of what getCalleeSavedRegs returns. This, however, does not work for us. Should it be the all-inclusive set of registers, including all aliases that are fully covered by the pristine bits from the register file, or a minimal set of registers that cover these bits? Anything that is in between?

[kparzysz]

New version, as per the discussion

[kparzysz]

Delete unused declaration.

[kparzysz]

I suppose we have agreed on this. Committed in r253600.

[MatzeB]

Accept + Close review that is already committed.

[MatzeB]

Accept + Close review that is already committed.

lib/CodeGen/MachineFunction.cpp

BitVector MachineFrameInfo::getPristineRegs(const MachineFunction &MF) const {
   // freely used and PEI will make sure they are saved.
   if (!isCalleeSavedInfoValid())
     return BV;
 
   for (const MCPhysReg *CSR = TRI->getCalleeSavedRegs(&MF); CSR && *CSR; ++CSR)
     BV.set(*CSR);
 
   // Saved CSRs are not pristine.
-  const std::vector<CalleeSavedInfo> &CSI = getCalleeSavedInfo();
-  for (std::vector<CalleeSavedInfo>::const_iterator I = CSI.begin(),
-         E = CSI.end(); I != E; ++I)
-    BV.reset(I->getReg());
+  for (auto &I : getCalleeSavedInfo())
+    for (MCSubRegIterator S(I.getReg(), TRI, true); S.isValid(); ++S)
+      BV.reset(*S);
 
   return BV;
 }
 
 unsigned MachineFrameInfo::estimateStackSize(const MachineFunction &MF) const {
   const TargetFrameLowering *TFI = MF.getSubtarget().getFrameLowering();
   const TargetRegisterInfo *RegInfo = MF.getSubtarget().getRegisterInfo();
   unsigned MaxAlign = getMaxAlignment();