Fix SROA for intrinsics

[Chad Rosier]

Hi Mandeep,
Thanks for working on this fix!  If you can give me some additional
information (either on-list or off), I can assist you with making a test
case and posting a patch to Phabricator.  We should be able to take one of
the existing SROA tests and throw in lifetime intrinsics to expose the
issue.

 Chad

> Hi,
>
>
>
> 1.       The existing code has a logical error where an elsif clause has
> been incorrectly written whereby it skips the subsequent elseif clauses.
>
> 2.       I observed that in the existing code after the SROA patch a lot
> of “ins” instructions whenever an intrinsic is seen.
> This generation of “ins” in order to load a 64 bit vector does not
> always result in efficient code.
>
>
>
> I am attaching the bitcode files for a simple testcase with and without my
> patch.
>
>
>
> Thanks,
>
> Mandeep
>
>
>
> From: Chandler Carruth [mailto:chandlerc at google.com]
> Sent: Sunday, January 25, 2015 6:57 PM
> To: Mandeep Singh Grang
> Cc: Commit Messages and Patches for LLVM
> Subject: Re: Fix SROA for intrinsics
>
>
>
> This looks generally like a good improvement, but it has no test cases or
> a clear explanation of the problem it is fixing. It needs those before it
> can go into the tree.
>
>
>
> On Sun, Jan 25, 2015 at 6:48 PM, Mandeep Singh Grang
> <mgrang at codeaurora.org <mailto:mgrang at codeaurora.org> > wrote:
>
> Hi,
>
>
>
> Can the owner for SROA please take a look into the attached patch?
>
>
>
> Also if you are fine with the patch could you please commit it since I do
> not have commit rights?
>
>
>
> Thanks,
>
> Mandeep
>
> From: Owen Anderson [mailto:resistor at mac.com]
> Sent: Wednesday, January 21, 2015 8:18 PM
> To: Mandeep Singh Grang
> Cc: llvm-commits; apazos at codeaurora.org <mailto:apazos at codeaurora.org>
> Subject: Re: Question about commit
> d4748bbd497b550a4e5db246c6708fcd6de542da
>
>
>
> Ok, patches welcome. :-)
>
>
>
> —Owen
>
>
>
> On Jan 21, 2015, at 8:00 PM, Mandeep Singh Grang <
> <mailto:mgrang at codeaurora.org> mgrang at codeaurora.org> wrote:
>
>
>
> Yes, in the scenario where we have an intrinsic and the following
> condition is true:
>
> else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser()))
>
>
>
> But the following condition in false:
>
> if (II->getIntrinsicID() != Intrinsic::lifetime_start &
>
>     II->getIntrinsicID() != Intrinsic::lifetime_end)
>
>
>
> In this case, all other subsequent else-if’s are skipped and the logic
> falls through to the “return true” and we see a lot of “ins”
> instructions being generated.
>
> But I think it should evaluate other else if’s since some of them may
> meet the condition and we may return false.
>
>
>
> We saw degradation in AArch64 cortex-a57 code due to this.
>
>
>
> We modified the code on our end to the following:
>
>
>
> IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser());
>
>
>
> <start of if else clauses>
>
> …
>
> else if (II && II->getIntrinsicID() != Intrinsic::lifetime_start &
>
>            II->getIntrinsicID() != Intrinsic::lifetime_end)
>
>
>
> <other else if’s>
>
>
>
> Making this change get us back the lost performance.
>
>
>
>
>
> Thanks,
>
> Mandeep
>
> From: Owen Anderson [ <mailto:resistor at mac.com> mailto:resistor at mac.com]
> Sent: Wednesday, January 21, 2015 6:30 PM
> To: Mandeep Singh Grang
> Cc: llvm-commits;  <mailto:apazos at codeaurora.org> apazos at codeaurora.org
> Subject: Re: Question about commit
> d4748bbd497b550a4e5db246c6708fcd6de542da
>
>
>
> Yes, it is intentional. Do you have a counter example where it results in
> undesirable behavior?
>
>
>
> -Owen
>
>
> On Jan 21, 2015, at 6:04 PM, Mandeep Singh Grang <
> <mailto:mgrang at codeaurora.org> mgrang at codeaurora.org> wrote:
>
> Hi Owen,
>
>
>
> In the following commit, we observed that with your else-if condition all
> the subsequent else-if’s are skipped.
>
> Could you please confirm whether this was your intent?
>
>
>
> commit d4748bbd497b550a4e5db246c6708fcd6de542da
>
> Author: Owen Anderson < <mailto:resistor at mac.com> resistor at mac.com>
>
> Date:   Thu Aug 7 21:07:35 2014 +0000
>
>
>
>     Fix a case in SROA where lifetime intrinsics could inhibit alloca
> promotion.  In
>
>     this case, the code path dealing with vector promotion was missing the
> explicit
>
>     checks for lifetime intrinsics that were present on the corresponding
> integer
>
>     promotion path.
>
>
>
> +++ b/lib/Transforms/Scalar/SROA.cpp
>
> @@ -1659,6 +1659,10 @@ static bool isVectorPromotionViableForSlice(
>
>        return false;
>
>      if (!I->isSplittable())
>
>        return false; // Skip any unsplittable intrinsics.
>
> +  } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser())) {
>
> +    if (II->getIntrinsicID() != Intrinsic::lifetime_start &&
>
> +        II->getIntrinsicID() != Intrinsic::lifetime_end)
>
> +      return false;
>
>    } else if (U->get()->getType()->getPointerElementType()->isStructTy())
> {
>
>      // Disable vector promotion when there are loads or stores of an FCA.
>
>      return false;
>
>
>
>
>
> Thanks,
>
> Mandeep
>
>
>
> <Fix_SROA_for_intrinsics.patch>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>