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 --------------
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
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