[llvm-commits] Please review: make bugpoint aware of blockaddress constant in global initializers

[Eli Friedman]

On Wed, Feb 8, 2012 at 2:56 PM, Daniel Reynaud <dreynaud at apple.com> wrote:
>
> On Feb 7, 2012, at 5:44 PM, Eli Friedman wrote:
>
>> On Tue, Feb 7, 2012 at 5:21 PM, Daniel Reynaud <dreynaud at apple.com> wrote:
>>> Even better with the actual patch.
>>>
>>>
>>>
>>> On Feb 7, 2012, at 5:20 PM, Daniel Reynaud wrote:
>>>
>>> This is a patch for http://llvm.org/bugs/show_bug.cgi?id=11919
>>>
>>> It seems to do the trick for me. When this particular configuration (a
>>> global initializer references a blockaddress from a function that has been
>>> sent to a different module) is not encountered, it should produce the same
>>> initializer pattern as usual (i.e., everything in the safe module).
>>>
>>> thanks,
>>> daniel
>>
>> +  bool globalInitUsesExternalBA(GlobalVariable* GV) {
>> +      if (!GV->hasInitializer())
>> +          return false;
>> +
>> +      Constant *I = GV->getInitializer();
>> +
>> +      // walk the values used by the initializer
>> +      // (and recurse into things like ConstantExpr)
>> +      std::vector<Constant*> Worklist;
>> +      Worklist.push_back(I);
>> +      while(!Worklist.empty()) {
>> +        Constant* V = Worklist.back();
>> +        Worklist.pop_back();
>> +
>> +        if (BlockAddress *BA = dyn_cast<BlockAddress>(V)) {
>> +          Function *F = BA->getFunction();
>> +          if (F->isDeclaration())
>> +            return true;
>> +        }
>> +
>> +        for (User::op_iterator i = V->op_begin(), e = V->op_end(); i != e; ++i)
>> +          if (Constant *C = dyn_cast<Constant>(*i))
>> +            Worklist.push_back(C);
>> +      }
>> +
>> +    return false;
>> +  }
>>
>> You need to make sure not to recurse into GlobalValues.
>
> ok
>
>>  You also
>> probably want a hashtable so you don't run into bad performance
>> characteristics with ConstantExprs.
>
> ok. I tried putting various things in the cache (everything, nothing, just ConstantExpr), and it did not have any noticeable impact on my test case (76s to reduce 459 functions).

I'm not really worried about the performance of normal cases; the
issue is that there are some pathological cases which can cause the
naive approach to to take exponential time.

>>
>> Also, weird indentation.
>>
>>
>> +  // Try to split the global initializers evenly
>> +  for (Module::global_iterator I = M->global_begin(), E = M->global_end();
>> +       I != E; ++I) {
>> +    GlobalVariable *GV = cast<GlobalVariable>(NewVMap[I]);
>> +    if (globalInitUsesExternalBA(I)) {
>> +      assert(!globalInitUsesExternalBA(GV) && "A global initializer references"
>> +          "functions in both the safe and the test module");
>>
>> I don't see what prevents this assertion from triggering…
>
> nothing indeed. If a global initializer references blockaddresses across modules, I don't think we can split.

True; we should at least give a proper error message, though.

-Eli