[LLVMdev] PR4174

[Eli Friedman]

On Fri, Aug 21, 2009 at 6:00 PM, Eli Friedman<eli.friedman at gmail.com> wrote:
> On Fri, Aug 21, 2009 at 5:47 PM, Jakub Staszak<kuba at gcc.gnu.org> wrote:
>>
>> On Aug 21, 2009, at 10:27 PM, Eli Friedman wrote:
>>
>>> On Fri, Aug 21, 2009 at 5:05 PM, Jakub Staszak<kuba at gcc.gnu.org> wrote:
>>>>
>>>> On Aug 21, 2009, at 10:02 PM, Eli Friedman wrote:
>>>>
>>>>> On Fri, Aug 21, 2009 at 4:53 PM, Jakub Staszak<kuba at gcc.gnu.org> wrote:
>>>>>>
>>>>>> On Aug 21, 2009, at 8:46 PM, Eli Friedman wrote:
>>>>>>
>>>>>>> On Fri, Aug 21, 2009 at 3:29 PM, Jakub Staszak<kuba at gcc.gnu.org>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On Aug 21, 2009, at 7:31 PM, Eli Friedman wrote:
>>>>>>>>
>>>>>>>>> On Fri, Aug 21, 2009 at 2:06 PM, Jakub Staszak<kuba at gcc.gnu.org>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> This patch fixes PR4174. Two test-cases included: original one from
>>>>>>>>>> bugzilla
>>>>>>>>>> and a little bit complicated made be myself.
>>>>>>>>>
>>>>>>>>> I think you want isSafeToSpeculativelyExecute rather than
>>>>>>>>> mayHaveSideEffects.  Otherwise, looks fine.
>>>>>>>>
>>>>>>>> With isSafeToSpeculativelyExecute "make check" fails on some tests,
>>>>>>>> i.e:
>>>>>>>> test/Transforms/LoopIndexSplit/SaveLastValue-2007-08-17.ll
>>>>>>>> because of PHI (isSafeToSpeculativelyExecute == false).
>>>>>>>
>>>>>>> isSafeToSpeculativelyExecute returns false for PHI nodes because it
>>>>>>> isn't really clear what to do in that case... here, it's safe, so
>>>>>>> isSafeToSpeculativelyExecute || isa<PHINode> is probably best.
>>>>>>
>>>>>> Another two "hacks" are needed: for BranchInst and for IntrinsicInst.
>>>>>>
>>>>>> Patch attached.
>>>>>
>>>>> IntrinsicInst isn't necessarily safe...
>>>>
>>>>
>>>> Without IntrinsicInst we break
>>>> test/Transforms/LoopIndexSplit/SplitValue-2007-08-24-dbg.ll.
>>>
>>> Checking for isa<DbgInfoIntrinsic> is safe.
>>
>> Ah, right :) Thanks for your help. I hope that everything is OK now.
>>
>> New patch attached.
>
> Looks fine, I think.  I should probably consider some changes to
> isSafeToSpeculativelyExecute, though...
>
> That said, I'm not entirely sure how this is supposed to interact with
> LoopIndexSplit::cleanBlock... I never really quite understood what
> it's doing.

Umm, make that "I'm not entirely sure whether you should be doing what
you're currently doing or using cleanBlock here".

-Eli