ShrinkWrap pass uses wrong csr

[Quuxplusone]

Bugzilla Link	PR42999
Status	NEW
Importance	P enhancement
Reported by	manjian (manjian2006@gmail.com)
Reported on	2019-08-14 17:39:48 -0700
Last modified on	2019-08-19 04:12:41 -0700
Version	trunk
Hardware	PC Linux
CC	francisvm@yahoo.com, htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, matze@braunis.de, quentin.colombet@gmail.com, westion717@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Index: lib/CodeGen/ShrinkWrap.cpp
===================================================================
--- lib/CodeGen/ShrinkWrap.cpp  (revision 359070)
+++ lib/CodeGen/ShrinkWrap.cpp  (working copy)
@@ -280,8 +280,8 @@
       // separately. An SP mentioned by a call instruction, we can ignore,
       // though, as it's harmless and we do not want to effectively disable tail
       // calls by forcing the restore point to post-dominate them.
-      UseOrDefCSR = (!MI.isCall() && PhysReg == SP) ||
-                    RCI.getLastCalleeSavedAlias(PhysReg);
+      UseOrDefCSR =
+          (!MI.isCall() && PhysReg == SP) || getCurrentCSRs(RS).count(PhysReg);
     } else if (MO.isRegMask()) {
       // Check if this regmask clobbers any of the CSRs.
       for (unsigned Reg : getCurrentCSRs(RS)) {

RCI.getLastCalleeSavedAlias(PhysReg) may work on many cases, but one target may
specify other CSRs in its frame lowering implementation. For example,
ARMFrameLowering.cpp has the following code:
  if (STI.isTargetWindows() &&
      WindowsRequiresStackProbe(MF, MFI.estimateStackSize(MF))) {
    SavedRegs.set(ARM::R4);
    SavedRegs.set(ARM::LR);
  }

It add R4 to CSR set. So I suggest ShrinkWrap should use the patch above, which
uses getCurrentCSRs to determine CSRs.

[Quuxplusone]

(In reply to manjian from comment #0)
> Index: lib/CodeGen/ShrinkWrap.cpp
> ===================================================================
> --- lib/CodeGen/ShrinkWrap.cpp    (revision 359070)
> +++ lib/CodeGen/ShrinkWrap.cpp    (working copy)
> @@ -280,8 +280,8 @@
>        // separately. An SP mentioned by a call instruction, we can ignore,
>        // though, as it's harmless and we do not want to effectively disable
> tail
>        // calls by forcing the restore point to post-dominate them.
> -      UseOrDefCSR = (!MI.isCall() && PhysReg == SP) ||
> -                    RCI.getLastCalleeSavedAlias(PhysReg);
> +      UseOrDefCSR =
> +          (!MI.isCall() && PhysReg == SP) ||
> getCurrentCSRs(RS).count(PhysReg);
>      } else if (MO.isRegMask()) {
>        // Check if this regmask clobbers any of the CSRs.
>        for (unsigned Reg : getCurrentCSRs(RS)) {
>
> RCI.getLastCalleeSavedAlias(PhysReg) may work on many cases, but one target
> may specify other CSRs in its frame lowering implementation. For example,
> ARMFrameLowering.cpp has the following code:
>   if (STI.isTargetWindows() &&
>       WindowsRequiresStackProbe(MF, MFI.estimateStackSize(MF))) {
>     SavedRegs.set(ARM::R4);
>     SavedRegs.set(ARM::LR);
>   }
>
> It add R4 to CSR set. So I suggest ShrinkWrap should use the patch above,
> which uses getCurrentCSRs to determine CSRs.

You can put this on phabricator to get more feedback or contribute to codebase.

[Quuxplusone]

I get no idea how to use it? Any help?

[Quuxplusone]

(In reply to manjian from comment #2)
> I get no idea how to use it? Any help?

It's a review tool for whole llvm project. https://reviews.llvm.org.
You can see document at http://llvm.org/docs/Phabricator.html. And so is
http://llvm.org/docs/DeveloperPolicy.html.