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

[Dan Gohman]

On Nov 8, 2010, at 8:31 AM, Duncan Sands wrote:

> 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.

>> @@ -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.

Dan