Fix SROA for intrinsics

[Mandeep Singh Grang]

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

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150128/9972e5a6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Fix_SROA_for_intrinsics.patch
Type: application/octet-stream
Size: 1117 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150128/9972e5a6/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sroa_without_fix.ll
Type: application/octet-stream
Size: 983 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150128/9972e5a6/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sroa_with_fix.ll
Type: application/octet-stream
Size: 1769 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150128/9972e5a6/attachment-0002.obj>