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

[Eli Friedman]

On Wed, Feb 8, 2012 at 5:18 PM, Daniel Reynaud <dreynaud at apple.com> wrote:
>
> On Feb 8, 2012, at 3:05 PM, Eli Friedman wrote:
>
>> 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.
>
> Now with a proper error message then.

r151115; sorry about the delay.

-Eli