[PATCH] D107454: [ARM] Change a couple of instances of LiveRegs.contains to !LiveRegs.available

Sjoerd Meijer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 6 06:55:32 PDT 2021


SjoerdMeijer accepted this revision.
SjoerdMeijer added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/ARM/Thumb1FrameLowering.cpp:588
   for (auto Reg : GPRsNoLRSP.set_bits()) {
-    if (!UsedRegs.contains(Reg)) {
       // Remember the first pop-friendly register and exit.
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > I have not looked very long at this (and the context how this is used), but if we want to find an available temporary, then the check:
> > 
> >   "the live physical registers UsedRegs does not contain Reg"
> > 
> > sounds about right to me.
> > 
> > So can you perhaps explain the problem and why this is wrong?
> The docs of contains() say it all:
>   /// Note: Returns false if just some sub registers are live, use available()
>   /// when searching a free register.
> 
> available() will also check for reserved regs, like r7 under the thumb1 test. The reason I was looking at this was because of the subreg liveness. RDA needed a number of fixes to make it correct. This case possibly does not matter as much, with GPR regs not having subregs, but available() seems like the more correct function to use.
Ah, thanks, makes sense!

Looks like a good fix to me!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107454/new/

https://reviews.llvm.org/D107454



More information about the llvm-commits mailing list