[PATCH] D13034: Do not mark CSRs as pristine in functions that have been shrink wrapped

hfinkel@anl.gov via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 25 05:13:44 PDT 2015


hfinkel added a comment.

In http://reviews.llvm.org/D13034#250665, @kbarton wrote:

> In http://reviews.llvm.org/D13034#250216, @MatzeB wrote:
>
> > Can you give some more context why this fails? I recently changed the PristineRegister definition to be independent of the place in the function so it wouldn't matter whether you are in the prologue or epilogue of the function. For the prologue/epilogue those saved CSRs should be marked as live-in values and therefore should be considered live by the RegisterScavenger/LivePhysRegs. Do you have an instance where this is not the case?
>
>
> The attached test case demonstrates how this fails.
>  Shrink wrapping identifies a new location for the save/restore blocks. Callee-saved register R30 is then moved to be saved/restored outside of the prologue. Later in the compilation, during Aggressive Anti-Dependence breaking, R30 is chosen to be used to break an anti-dependence in a block *after* the restore location identified by shrink wrapping. This clobbers the value in R30.


The issue here maybe an assumption that the AggressiveAntiDepBreaker makes about how PristineRegisters work, specifically, in AggressiveAntiDepBreaker::StartBlock we have this code:

  // Mark live-out callee-saved registers. In a return block this is
  // all callee-saved registers. In non-return this is any
  // callee-saved register that is not saved in the prolog.
  const MachineFrameInfo *MFI = MF.getFrameInfo();
  BitVector Pristine = MFI->getPristineRegs(MF);
  for (const MCPhysReg *I = TRI->getCalleeSavedRegs(&MF); *I; ++I) {
    unsigned Reg = *I;
    if (!IsReturnBlock && !Pristine.test(Reg)) continue;
    for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
      unsigned AliasReg = *AI;
      State->UnionGroups(AliasReg, 0);
      KillIndices[AliasReg] = BB->size();
      DefIndices[AliasReg] = ~0u;
    }
  }

So, in a return block, it is marking all callee-saved registers as live-out, and in a non-return block, it is marking as live-out all callee-saved registers that are pristine (and, thus, should not be used). The assumption here, obviously, is that the restore point for non-pristine callee-saved registers will be in the return block. With shrink-wrapping enabled, this clearly won't be the case.

I'll also note that, prior to Matthias's change in r238524, we were collecting pristine registers per-MBB, not per function. Even then, however, it is not clear to me that the code would have been correct for shrink wrapping unless the set of pristine registers was maximal in blocks with the restore point.

Also worth noting is that in CriticalAntiDepBreaker::StartBlock we have very-similar logic:

  // Mark live-out callee-saved registers. In a return block this is
  // all callee-saved registers. In non-return this is any
  // callee-saved register that is not saved in the prolog.
  const MachineFrameInfo *MFI = MF.getFrameInfo();
  BitVector Pristine = MFI->getPristineRegs(MF);
  for (const MCPhysReg *I = TRI->getCalleeSavedRegs(&MF); *I; ++I) {
    if (!IsReturnBlock && !Pristine.test(*I)) continue;
    for (MCRegAliasIterator AI(*I, TRI, true); AI.isValid(); ++AI) {
      unsigned Reg = *AI;
      Classes[Reg] = reinterpret_cast<TargetRegisterClass *>(-1);
      KillIndices[Reg] = BBSize;
      DefIndices[Reg] = ~0u;
    }
  }

these parts of the code are many years old, and perhaps date from a time when relevant parts of the infrastructure were different.

Looking at the function just prior to anti-dependency breaking (which is post-RA scheduling), %X30 is certainly marked as live-in to those blocks that are predecessors to the now-conditionally-executed prologue block (which is the same block containing the epilogue). However, it is not in the live-in set of any other block in the function, and thus the problem. In the non-return blocks, because %X30 is not in the set of pristine registers, and it is not marked live-in anywhere else, the anti-dependency breakers will assume it to be available for allocation. As far as I can tell, the register scavenger will suffer from a similar problem.

Kit, while your fix is conservatively correct, I'd like to find a better way. PrologEpilogInserter contains code in a function called updateLiveness to add Live-In registers to blocks based on the selected save/restore points. You could try updating it to mark pristine registers outside the save/restore region as live-in as well. Another option is the teach the anti-dependency breakers about save/restore points, and let them use dominance queries to determine if they need to consider all callee-saved registers as pristine (just as they currently do in return blocks). I imagine that checking whether a block is dominated by the save point and not dominated by the restore point would do the trick.


http://reviews.llvm.org/D13034





More information about the llvm-commits mailing list