[LLVMdev] PR4174

[Eli Friedman]

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.

-Eli