[PATCH][x86] Mark TEST8ri_NOREX hasSideEffects=0

Akira Hatanaka ahatanak at gmail.com
Thu Jul 10 11:11:48 PDT 2014


I committed the patch with a comment and test case (r212747).

Thank you for the review and feedback.


On Wed, Jul 9, 2014 at 6:56 PM, Eric Christopher <echristo at gmail.com> wrote:

> On Wed, Jul 9, 2014 at 6:54 PM, Bob Wilson <bob.wilson at apple.com> wrote:
> >
> >> On Jul 9, 2014, at 6:52 PM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>
> >> On Wed, Jul 9, 2014 at 6:48 PM, Bob Wilson <bob.wilson at apple.com>
> wrote:
> >>>
> >>>> On Jul 9, 2014, at 6:36 PM, Eric Christopher <echristo at gmail.com>
> wrote:
> >>>>
> >>>> Might as well add the testcase since we're sure that's the right
> behavior yes?
> >>>
> >>> Sure, that’s a good idea.
> >>>
> >>>>
> >>>> Also I'm probably missing something about having the pseudo as
> >>>> hasSideEffects = 0 when the instruction the pseudo is representing
> >>>> does have side effects. Mind explaining?
> >>>
> >>> It’s defined inside a let-block with Defs = [EFLAGS]. Are there other
> side effects besides that?
> >>
> >> Ah, I see. No, no other side effects than that, and that's the same
> >> for all test instructions on x86. And since hasSideEffects counts the
> >> operands and I'm guessing it counts the let Defs = as operands then
> >> there are no other side effects. Whee.
> >>
> >> Probably should move the hasSideEffects = 0 up then to the Defs?
> >
> > All of the other instructions there have patterns, so tablegen will
> infer that they don’t have side effects. TEST8ri_NOEREX is the only one
> without a pattern.
>
> Whee.
>
> Akira: How about a comment then with the testcase?
>
> -eric
>
> >
> >>
> >> -eric
> >>
> >>>
> >>>>
> >>>> -eric
> >>>>
> >>>> On Wed, Jul 9, 2014 at 6:27 PM, Bob Wilson <bob.wilson at apple.com>
> wrote:
> >>>>> LGTM.
> >>>>>
> >>>>> Even though the original issue no only occurs, the changes here are
> still the right thing to do. For the record, the original test was:
> >>>>>
> >>>>> target datalayout =
> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f\
> >>>>>
> 32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32\
> >>>>> :64-S128"
> >>>>> target triple = "x86_64-apple-macosx10.9.0"
> >>>>>
> >>>>> define i32 @check_flag(i32 %flags, ...) nounwind {
> >>>>> entry:
> >>>>> %and = and i32 %flags, 512
> >>>>> %tobool = icmp eq i32 %and, 0
> >>>>> br i1 %tobool, label %if.end, label %if.then
> >>>>>
> >>>>> if.then:                                          ; preds = %entry
> >>>>> br label %if.end
> >>>>>
> >>>>> if.end:                                           ; preds = %entry,
> %if.then
> >>>>> %hasflag = phi i32 [ 1, %if.then ], [ 0, %entry ]
> >>>>> ret i32 %hasflag
> >>>>> }
> >>>>>
> >>>>>> On Jul 7, 2014, at 3:07 PM, Akira Hatanaka <ahatanak at gmail.com>
> wrote:
> >>>>>>
> >>>>>> This patch marks the pattern-less pseudo-instruction TEST8ri_NOEREX
> as hasSIdeEffects=0 and adds a case clause for it in
> X86InstrInfo::shouldScheduleAdjacent to enable macro-fusion.
> >>>>>>
> >>>>>> The original intent of this patch was to fix a bug where a testb
> instruction was scheduled apart from the je instruction that reads its
> result (this bug can be reproduced with r192750). It seems that this bug no
> longer occurs after the node ordering of selection DAG nodes was fixed, so
> I wasn't able to come up with the a test case.
> >>>>>>
> >>>>>> <test8rinorex1.patch>_______________________________________________
> >>>>>> llvm-commits mailing list
> >>>>>> llvm-commits at cs.uiuc.edu
> >>>>>> http://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/20140710/0fa036ad/attachment.html>


More information about the llvm-commits mailing list