<div dir="ltr"><div><div><div style>Sure, I'm happy with this approach too.</div><div><br></div><div>+// \brief Collects GV's initializer elements in Set.</div><div>+static GlobalVariable *</div><div>+collectUsedGlobalVariables(const Module &M, const char *Name,</div>
<div>+ SmallPtrSet<GlobalValue *, 8> &Set) {</div></div><div><br></div><div style>I know what this is doing, but I'd appreciate a better comment for when I'm look at this years from now. How about:</div>
<div style> // Given a @llvm.used global, collect the initializer elements in Set.</div><div style>? As written I can see myself getting confused by "Used" in the function name and thinking of use-lists.</div>
<div>
<br></div><div>+static bool</div><div>+hasUseOtherThanLLVMUsed(GlobalAlias &GA,</div><div>+ const SmallPtrSet<GlobalValue *, 8> &Used,</div><div>+ const SmallPtrSet<GlobalValue *, 8> &CompilerUsed) {</div>
<div>+ if (GA.use_empty()) // No use at all.</div><div>+ return false;</div><div>+</div><div>+ if (!GA.hasOneUse()) // > 1 use and at most one is in llvm.used.</div><div>+ return true;</div></div><div><br></div>
<div style>Hm? It could have one use which is llvm.used.</div><div style><br></div><div style><div>+ if (Used.count(&GA) || CompilerUsed.count(&GA))</div><div>+ return false;</div><div>+ return true;</div><div>
<br></div><div style>Optional: Consider using</div><div style>return !Used.count(&GA) && !CompilerUsed.count(&GA);</div><div style><br></div><div style><div>+static bool hasMoreThanOneUseOtherThanLLVMUsed(</div>
<div>+ GlobalValue &V, const SmallPtrSet<GlobalValue *, 8> &Used,</div><div>+ const SmallPtrSet<GlobalValue *, 8> &CompilerUsed) {</div><div>+ unsigned N = 2;</div><div>+ if (Used.count(&V) || CompilerUsed.count(&V))</div>
<div>+ ++N;</div><div>+ return V.hasNUsesOrMore(N);</div><div>+}</div><div><br></div><div style>I don't think that logic is right when V is contained in both Used and CompilerUsed. You'll want to bump N by two.</div>
<div style><br></div><div style><div>Say, you've got a lot of functions that take Used and CompilerUsed. Any interested in grouping these into a utility class that holds those two as members? Or, just one member?</div>
<div><br></div></div><div style><div>+ GlobalVariable *NV =</div><div>+ new GlobalVariable(M, ATy, false, llvm::GlobalValue::AppendingLinkage,</div><div>+ llvm::ConstantArray::get(ATy, UsedArray), "");</div>
<div><br></div><div style>Optional: You can use V.getParent() and remove 'M' from the signature of this function.</div><div style><br></div><div style>+bool hasUsesToReplace(GlobalAlias &AS,<br></div><div style>
<br></div><div style>"AS"? Did you mean "GA"?</div></div></div><div style><br></div><div style><div><div>+ for (SmallPtrSet<GlobalValue*, 8>::iterator I = Used.begin(), E = Used.end();</div><div>
+ I != E; ++I)</div><div>+ CompilerUsed.erase(*I);</div></div><div><br></div><div style>Ah okay, I see now why you previously assumed they're mutually exclusive. The problem is that this is an unstated assumption on their interfaces. This makes me want you to put them in a class even more, so that you can have documented invariants on the members.</div>
<div style><br></div><div style><div>+ if (Used.count(J)) {</div><div>+ Used.erase(J);</div><div>+ Used.insert(Target);</div><div>+ }</div><div><br></div><div style>if (Used.erase(J)) {</div><div style>
Used.insert(Target);</div><div style>}</div><div style><br></div><div style>Same for CompilerUsed right after it.</div></div></div><div style><br></div></div><div style>Nick</div><div><br></div>On 6 May 2013 06:16, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>>> I ended implementing two options. The first one (add-back.patch)<br>
>> looked the simplest at first: Just call replaceAllUsesWith and patch<br>
>> llvm.used back to its previous state. The main problem in here is<br>
>> detecting if any other changes were made. We have to walk the<br>
>> constant's uses to find out.<br>
>><br>
>> Since we ale walking the uses anyway, another option (cow.patch) is to<br>
>> duplicate a bit of the copy on write logic in replaceAllUsesWith to<br>
>> special case use chains that end up in llvm.used. In the end I think I<br>
>> like this one a bit more. What do you think?<br>
><br>
><br>
> Yes, I also prefer cow.patch. Thanks for looking into this!<br>
<br>
</div>Sorry, I noticed there was a third option:<br>
* Call replaceAllUsesWith only if there uses other than the ones from<br>
llvm.used and llvm.compiler_used.<br>
* Rebuild llvm.used and llvm.compiler_used only at the end of<br>
OptimizeGlobalAliases.<br>
<br>
This has the advantage that it is easy to extend to handle the extra<br>
optimization where we rename the target. With that enabled clang<br>
compiles<br>
<br>
extern "C" {<br>
__attribute__((used)) static void foo() {}<br>
}<br>
<br>
to<br>
<br>
@llvm.used = appending global [1 x i8*] [i8* bitcast (void ()* @foo to<br>
i8*)], section "llvm.metadata"<br>
define internal void @foo() #0 {<br>
entry:<br>
ret void<br>
}<br>
<br>
Which is really cool. LLVM understands the hack clang did to keep the<br>
name foo visible and removes it :-)<br>
<br>
> Nick<br>
<br>
Cheers,<br>
Rafael<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>