[PATCH] D46315: [RegUsageInfoCollector] Fix handling of callee saved registers with CSR optimization.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.

Thanks for double checking. Now, I don't think some part of the patch makes sense. See my inline comment.
Basically, we should have to augment whatever determineCalleeSaved gives us, otherwise that means this information is just flawed.

> The target returns a list of registers from determineCalleeSaves that does not include any aliases.

The default implementation should include the aliases (it relies on isPhysRegModified).
If we don't have the aliases in here, a lot of passes would be broken, like shrink-wrapping or PEI.

> It seems that we must therefore build this set so that it includes any fully saved/restored register.

Yes, that's what it should do.

> I have added the subregs already, but it seems we should also check which super-regs end up as fully included, right?

This should already happen in the default implementation of determineCalleeSaves.

> Should we make a new function that does this work, since we need this also in the MachineVerifier, like determineCalleeSavesWithAliases? Maybe this could be a static method in RegUsageInfoCollector?

No, we shouldn't need that.



================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:117
+        SavedRegs.set(*SR);
+  }
+
----------------
That loop shouldn't be required. determineCalleeSaves should already mark those.


================
Comment at: lib/CodeGen/RegUsageInfoCollector.cpp:149
+          << " function optimized for not having CSR.\n");
   }
 
----------------
jonpa wrote:
> qcolombet wrote:
> > This logging is not very useful now. I would get rid of it.
> Do you want to remove this entire clause of just the dbgs() output?
> 
> I might object a little to removing this, since I have made SystemZ tests that checks for this, for instance to check that the return register is saved even on such a function:
> 
> ```
> ; DBG: fun3 function optimized for not having CSR
> ; CHECK-LABEL: fun3
> ​; CHECK: stmg    %r14, %r15, 112(%r15)
> ​; CHECK: lr      %r14
> ​; CHECK: a       %r14
> ​; CHECK: lmg     %r14, %r15, 112(%r15)
> ​; CHECK: br      %r14
> 
> ```
> It also seems useful to IPRA to have the statistic around...
> 
To me the logging is not useful anymore because there is no change of behavior in the past itself based on isSafeForNoCSROpt.
It looks to me that we are printing this because we expect determineCalleeSaved to do the right thing with it, but that's a big assumption :).


https://reviews.llvm.org/D46315