[llvm-commits] [llvm] r118410 - in /llvm/trunk: lib/Transforms/IPO/FunctionAttrs.cpp test/Transforms/FunctionAttrs/2008-10-04-LocalMemory.ll

[Duncan Sands]

Hi Dan,

>>> Make FunctionAttrs use AliasAnalysis::getModRefBehavior, now that it
>>> knows about intrinsic functions.
>>
>> I always took care to *not* have function attributes make any use of
>> alias analysis.  Partly this is to have the function attributes pass
>> stay fast, partly from a feeling that function attributes should be
>> layered below alias analysis not on top of it, and partly because using
>> alias analysis doesn't (or didn't in the past) win you much.  That said,
>> I can understand the temptation to have it use alias analysis - but what
>> is the cost/benefit ratio?
>
> FunctionAttrs' PointsToLocalOrConstantMemory was basically just a slightly
> smarter version of BasicAliasAnalysis' pointsToConstantMemory, so I ported
> the additional features over (which is useful on its own). Subsequently, the
> FunctionAttrs code was redundant. Also, using AliasAnalysis gives
> FunctionAttrs the ability to take advantage of TBAA information without
> much extra effort.
>
> I see that there is a conceptual layering issue: FunctionAttrs uses AA,
> and AA uses the results of FunctionAttrs. However, it's the same problem
> whether FunctionAttrs duplicates BasicAliasAnalysis' code or calls it,
> so I don't see a strong reason to prefer duplicating.
>
> For speed, the BasicAA implementation is basically the same as the
> FunctionAttrs one (except the recursion check, which I'll address), and
> the TBAA implementation is cheap, so I don't expect there will be a
> big difference.

fair enough, thanks for the explanation.

>>> @@ -167,26 +171,35 @@
>>>         // Some instructions can be ignored even if they read or write memory.
>>>         // Detect these now, skipping to the next instruction if one is found.
>>>         CallSite CS(cast<Value>(I));
>>> -      if (CS&&   CS.getCalledFunction()) {
>>> +      if (CS) {
>>>           // Ignore calls to functions in the same SCC.
>>> -        if (SCCNodes.count(CS.getCalledFunction()))
>>> +        if (CS.getCalledFunction()&&   SCCNodes.count(CS.getCalledFunction()))
>>> +          continue;
>>
>> Shouldn't this be:
>>    if (!CS.getCalledFunction() || SCCNodes.count(CS.getCalledFunction()))
>> ?
>
> I don't think so. Indirect calls need to be handled conservatively,
> since the callee may not be in the current SCC.

You are correct.  I misread the effect of the change - previously this case was
handled conservatively too.

Ciao,

Duncan.