[PATCH] D31188: [AntiDepBreaker] Do not use getPristineRegs for marking live registers.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp:175
     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;
+    if (MFI.isCalleeSavedInfoValid() &&
+        (IsReturnBlock || BB->isLiveIn(Reg) || !HasSlot(Reg))) {
----------------
MatzeB wrote:
> I don't think this pass will (or should) be used before the prolog epilog inserter so isCalleeSavedInfoValid() should always be true and you could simply assert on it.
I didn't assert on this, only because of https://github.com/llvm-mirror/llvm/blob/6095a7948d32d712811e8c2ab2190acf8e66a8bc/lib/CodeGen/PrologEpilogInserter.cpp#L66

It appears that CSInfo is valid only if doSpillCalleeSavedRegs is executed.

Is it always the case where usesPhysRegsForPEI is necessary for post-RA scheduling?


================
Comment at: llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp:176
+    if (MFI.isCalleeSavedInfoValid() &&
+        (IsReturnBlock || BB->isLiveIn(Reg) || !HasSlot(Reg))) {
+      for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
----------------
MatzeB wrote:
> Adding `isLiveIn()` here looks wrong given that the code is already adding the live ins of the successors above (which is the way to compute the live-outs of this block). This should already be fine if the code is walking the block backwards afterwards.
> 
> Seeing this I would assume we walk the blocks backwards and starting with the live-outs so adding the live-ins would not make sense.
Looking at https://github.com/llvm-project/llvm-project/blob/master/llvm/lib/CodeGen/PostRASchedulerList.cpp#L331

The blocks are just traversed without an explicit order.

I'll take a closer look at StartBlock. I was more or less confused by its intention.


https://reviews.llvm.org/D31188