Fix SROA for intrinsics

[Grang, Mandeep Singh]

Hi chandlerc,

Just wanted to run this patch by you and confirm if you are fine with 
merging this:
http://reviews.llvm.org/D7240

Thanks,
Mandeep

On 1/28/2015 12:38 PM, Chad Rosier wrote:
> 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
>>
>