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. -Jakub
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. -Eli
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. -Jakub -------------- next part -------------- A non-text attachment was scrubbed... Name: pr4174-3.patch Type: application/octet-stream Size: 2943 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090821/99b3317e/attachment.obj> -------------- next part --------------