Re: [PATCH 1/2] [libata] sata_mv: Remove PCI dependency

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Jeff Garzik wrote:
>>> saeed bishara wrote:
>>>> -    if (unlikely(irq_stat & PCI_ERR)) {
>>>> +    if (unlikely(irq_stat & PCI_ERR) && HAS_PCI(host)) {
>>>>          mv_pci_error(host, mmio);
>>>>          handled = 1;
>>>>          goto out_unlock;    /* skip all other HC irq handling */
>>> the unlikely() should cover the entire expression.
>>
>> Hmm... I also sometimes hesitate about these things.  I think when the
>> first condition is unlikely but the latter isn't so, it's better to tell
>> the compiler that the first part is unlikely and let it figure out the
>> rest.  Another thing is using unlikely on the return value of helper
>> function like the following.
>>
>> static bool test_some_condition(arg)
>> {
>>     return unlikely(unlikely_test_on_arg(arg));
>> }
>>
>> I haven't looked at the generated code yet but hope the compiler can
>> DTRT if the function body is visible when compiling.  Has anyone played
>> with these?
> 
> Think about the question being answered -- this is branch prediction.
> After all tests in an 'if' are complete, the question is:  do we take
> this branch or not?  (yes/no)

Well, if (a && b) is not a single branch.  It's two (or more) branches.
 Let's say a is unlikely but b is; then with unlikely(a && b), the
compiler do something like the following.

	jmp L0 if !a
	jmp L0 if !b
	jmp L1
.L0	continue excution after if block

---------
cold unlikely block
.L1	body of if

Or some other form.  With (unlikely(a) && b), the compiler knows the
optimal form is...

	jmp L1 if a
.L0	continue excution after if block

---------
cold unlikely block
.L1	jmp L0 if !b
	body of if

> And while it is valid to note unlikely() as done above, it doesn't
> necessarily give the compiler the best idea about how to predict the
> ultimate end result of all the tests, which is the decision on whether
> or not to take the branch.

It actually gives the compiler better information to optimize with and
test conditions in if() can get quite involved and it also gives better
idea of what the writer was thinking when writing the likely/unlikely
when the code is read later.

-- 
tejun