[llvm] r237593 - Exploit dereferenceable_or_null attribute in LICM pass

[Chandler Carruth]

I'm not at all concerned about the time it took to respond, that was
awesome! =]

Over the past few days, it's been a matter of isolating one after another
regression. Not sure the last time we actually had been able to extensively
test top-of-tree and have it go all the way through. So my worry is with
low-confidence forward fixes. Those have a higher risk than reverts. I'm
not saying *this* fix is actually a bad idea, I'm just trying to encourage
lots of folks to be cautious about fixing forward without really high
confidence that the fix is both correct and reasonably complete.

-Chandler

On Thu, May 21, 2015 at 8:43 PM Philip Reames <listmail at philipreames.com>
wrote:

>  Chandler,
>
> I've been busy the last few days and not paying the most attention to LLVM
> happenings, but I'm a bit confused.  This change had been in for several
> days and, as far as I know, it hadn't broken the bots or anything.  A
> problem report to fix time of just under 3 hours (counting from Nick's
> email) seems entirely within normal practice.  What am I missing here?
>
> For the record, my fix landed at about the same time as your email.  If
> you'd like to revert both, I wouldn't actively oppose it.  I just haven't
> seen any need so far.
>
> Philip
>
>
> On 05/21/2015 07:19 PM, Chandler Carruth wrote:
>
> Given the very fragile state of the tree over the past few days, I'd
> suggest reverting first and then reapplying when you have the fix. If it
> ends up easy, great, but this way we can hopefully make it a bit farther in
> testing.
>
> On Thu, May 21, 2015 at 5:31 PM Philip Reames <listmail at philipreames.com>
> wrote:
>
>>  Sanjoy is out on vacation, but I'm going to look into this.  I'll be
>> offline for about an hour, but should be able to start investigating by
>> around 7pm.  On the surface, it looks like it should be an easy fix.
>> Hopefully, I'll have this resolved within a couple of hours.  Sorry for the
>> breakage.
>>
>> Philip
>>
>>
>> On 05/21/2015 04:38 PM, Nick Lewycky wrote:
>>
>>  On 18 May 2015 at 11:07, Sanjoy Das <sanjoy at playingwithpointers.com>
>> wrote:
>>
>>> Author: sanjoy
>>> Date: Mon May 18 13:07:00 2015
>>> New Revision: 237593
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=237593&view=rev
>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D237593-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=EZRvoJai0V1ZEULFLl3UFBFw3GpoHtruFDOok84hYrI&s=4QqYr7GqSrZjVxTSY4zWHbjeCHYmt-2bkaDpOtpbsM8&e=>
>>> Log:
>>> Exploit dereferenceable_or_null attribute in LICM pass
>>>
>>> Summary:
>>> Allow hoisting of loads from values marked with dereferenceable_or_null
>>> attribute. For values marked with the attribute perform
>>> context-sensitive analysis to determine whether it's known-non-null or
>>> not.
>>>
>>
>>  This commit caused PR23608, see the testcase in that PR.
>>
>>  -/// Only sink or hoist an instruction if it is not a trapping
>>> instruction
>>> +/// Only sink or hoist an instruction if it is not a trapping
>>> instruction,
>>> +/// or if the instruction is known not to trap when moved to the
>>> preheader.
>>>  /// or if it is a trapping instruction and is guaranteed to execute.
>>> -///
>>> -static bool isSafeToExecuteUnconditionally(const Instruction &Inst,
>>> +static bool isSafeToExecuteUnconditionally(const Instruction &Inst,
>>>                                             const DominatorTree *DT,
>>> +                                           const TargetLibraryInfo *TLI,
>>>                                             const Loop *CurLoop,
>>>                                             const LICMSafetyInfo
>>> *SafetyInfo) {
>>> -  // If it is not a trapping instruction, it is always safe to hoist.
>>> -  if (isSafeToSpeculativelyExecute(&Inst))
>>> +  const Instruction *CtxI =
>>> CurLoop->getLoopPreheader()->getTerminator();
>>>
>>
>>  A Loop* is not guaranteed to have a preheader, in which case
>> CurLoop->getLoopPreheader() will return null.
>>
>>  Nick
>>
>>
>>> +  if (isSafeToSpeculativelyExecute(&Inst, CtxI, DT, TLI))
>>>      return true;
>>>
>>>    return isGuaranteedToExecute(Inst, DT, CurLoop, SafetyInfo);
>>>
>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing listllvm-commits at cs.uiuc.eduhttp://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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150522/ebb1cec9/attachment.html>