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

[Daniel Reynaud]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bugpoint.patch
Type: application/octet-stream
Size: 3236 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120208/ea803133/attachment.obj>