[llvm-commits] [llvm] r60608 - in /llvm/trunk: lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86Subtarget.cpp lib/Transforms/Scalar/LoopStrengthReduce.cpp test/CodeGen/X86/loop-strength-reduce-2.ll test/CodeGen/X86/loop-strength-reduce-3.ll test/CodeGen/X86/loop-strength-reduce.ll

[Evan Cheng]

On Dec 8, 2008, at 2:02 PM, Dale Johannesen wrote:

>
> On Dec 8, 2008, at 1:54 PMPST, Evan Cheng wrote:
>>>>
>>>> Any plan to change the code? It doesn't makes sense to perform the
>>>> expensive checks first.
>>>
>>> It makes even less sense to make the code less comprehensible, and
>>> possibly break it (as you have seen), for a microoptimization.
>>> No, I
>>> think this is a good use of factoring, and I don't plan to change  
>>> it.
>>>
>>>> What about the first part of my comment? If PIC && !DirectCall, is
>>>> PIC
>>>> base ever unnecessary? From what I have see it's needed even if the
>>>> linkage is weak, common, etc.
>>>
>>> Those cases are picked up by RequiresExtraLoad.
>>
>> I don't understand your argument. Why can't we satisfy both
>> constraints?
>
> There has been no mention of "constraints" so far, what do you mean?

Fast and clear (and correct).

>
>
>> We know when isDirectCall this is always false, we also
>> know it's always true in PIC mode (at least for Darwin). How is this
>> less clear?
>
> You're losing the separation between "things handled by
> RequiresExtraLoad"
> and "things not handled that way".  There is overlap the way you've
> got it.

Why does that matter? The purpose of of the function is  not testing  
"requires an extra load".  Why insisting on testing that part first?  
Is it not true in PIC mode accessing GV always require a register? Is  
it not true when isDirectCall is true this should return false?

Looking at your code:

   if (GVRequiresExtraLoad(GV, TM, isDirectCall))
     return true;
   // Code below here need only consider cases where GVRequiresExtraLoad
   // returns false.
   if (TM.getRelocationModel() == Reloc::PIC_)
     return !isDirectCall &&
       (GV->hasInternalLinkage() || GV->hasExternalLinkage());
   return false;

If you don't look into GVRequiresExtraLoad, then you wouldn't see that  
weak, common, externalweak, and linkonce linkages were handled by  
GVRequiresExtraLoad. That's why the checks for internal and external  
linkages look strange.

EVan

>
>
>> {
>>  if (isDirectCall)
>>    return false;
>>  if (TM.getRelocationModel() == Reloc::PIC_) {
>>    if (isTargetDarwin())
>>      return true;
>>    else if (isTargetElf())
>>      ...
>>    else ...
>>      ...
>>  }
>>  return GVRequiresExtraLoad(GV, TM, false);
>> }
>>
>> Evan
>>
>>>
>>>
>>> _______________________________________________
>>> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits