On Fri, 2013-12-20 at 16:31 +0530, Anshuman Khandual wrote:
> On 12/09/2013 11:51 AM, Michael Ellerman wrote:
> > On Wed, 2013-04-12 at 10:32:40 UTC, Anshuman Khandual wrote:
> >> +
> >> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL) {
> >> + /* XL-form instruction */
> >> + if (instr_is_branch_xlform(*addr)) {
> >> +
> >> + /* LR should be set */
> >> + if (is_branch_link_set(*addr)) {
> >> + /*
> >> + * Conditional and unconditional
> >> + * branch to CTR.
> >> + */
> >> + if (is_xlform_ctr(*addr))
> >> + result = true;
> >> +
> >> + /*
> >> + * Conditional and unconditional
> >> + * branch to LR.
> >> + */
> >> + if (is_xlform_lr(*addr))
> >> + result = true;
> >> +
> >> + /*
> >> + * Conditional and unconditional
> >> + * branch to TAR.
> >> + */
> >> + if (is_xlform_tar(*addr))
> >> + result = true;
> >
> > What other kind of XL-Form branch is there?
>
> I am not sure. Do you know of any ?
That was my point. There are no other types, so you can just do:
if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL)
if (instr_is_branch_xlform(*addr) && is_branch_link_set(*addr))
return true;
> >> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_COND) {
> >> +
> >> + /* I-form instruction - excluded */
> >> + if (instr_is_branch_iform(*addr))
> >> + goto out;
> >> +
> >> + /* B-form or XL-form instruction */
> >> + if (instr_is_branch_bform(*addr) ||
> >> instr_is_branch_xlform(*addr)) {
> >> +
> >> + /* Not branch always */
> >> + if (!is_bo_always(*addr)) {
> >> +
> >> + /* Conditional branch to CTR register */
> >> + if (is_bo_ctr(*addr))
> >> + goto out;
> >
> > We might have discussed this but why not?
>
> Did not get that, discuss what ?
Why are we saying a conditional branch to the CTR is not a conditional branch?
It is conditional, so I think it should be included.
> >> +
> >> + /* CR[BI] conditional branch with static hint */
> >
> > A conditional branch with a static hint is still a conditional branch?
>
> No its not.
Yes it is?
In fact they could be very interesting branches. Because the compiler or
programmer has statically hinted them, if the hint is wrong they may be a major
source of branch midpredicts.
> >> + if (is_bo_crbi_off(*addr) ||
> >> is_bo_crbi_on(*addr)) {
> >> + if (is_bo_crbi_hint(*addr))
> >> + goto out;
> >> + }
> >> +
> >> + result = true;
> >> + }
> >> + }
> >> + }
> >> +out:
> >> + return result;
> >> +}
> >> + } else {
> >> + /*
> >> + * Userspace address needs to be
> >> + * copied first before analysis.
> >> + */
> >> + pagefault_disable();
> >> + ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
> >
> > I suspect you borrowed this incantation from the callchain code. Unlike that
> > code you don't fallback to reading the page tables directly.
> >
> > I'd rather see the accessor in the callchain code made generic and have you
> > call it here.
>
> You have mentioned to take care of this issue yourself.
Yes I will.
cheers
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
