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

[Jeff Garzik <jeff@garzik.org>]

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)

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.

	Jeff