[patch] Don't rauw an alias in llvm.used (v2)

[Nick Lewycky]

Sure, I'm happy with this approach too.

+// \brief Collects GV's initializer elements in Set.
+static GlobalVariable *
+collectUsedGlobalVariables(const Module &M, const char *Name,
+                           SmallPtrSet<GlobalValue *, 8> &Set) {

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:
  // Given a @llvm.used global, collect the initializer elements in Set.
? As written I can see myself getting confused by "Used" in the function
name and thinking of use-lists.

+static bool
+hasUseOtherThanLLVMUsed(GlobalAlias &GA,
+                        const SmallPtrSet<GlobalValue *, 8> &Used,
+                        const SmallPtrSet<GlobalValue *, 8> &CompilerUsed)
{
+  if (GA.use_empty()) // No use at all.
+    return false;
+
+  if (!GA.hasOneUse()) // > 1 use and at most one is in llvm.used.
+    return true;

Hm? It could have one use which is llvm.used.

+  if (Used.count(&GA) || CompilerUsed.count(&GA))
+    return false;
+  return true;

Optional: Consider using
return !Used.count(&GA) && !CompilerUsed.count(&GA);

+static bool hasMoreThanOneUseOtherThanLLVMUsed(
+    GlobalValue &V, const SmallPtrSet<GlobalValue *, 8> &Used,
+    const SmallPtrSet<GlobalValue *, 8> &CompilerUsed) {
+  unsigned N = 2;
+  if (Used.count(&V) || CompilerUsed.count(&V))
+    ++N;
+  return V.hasNUsesOrMore(N);
+}

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.

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?

+  GlobalVariable *NV =
+      new GlobalVariable(M, ATy, false,
llvm::GlobalValue::AppendingLinkage,
+                         llvm::ConstantArray::get(ATy, UsedArray), "");

Optional: You can use V.getParent() and remove 'M' from the signature of
this function.

+bool hasUsesToReplace(GlobalAlias &AS,

"AS"? Did you mean "GA"?

+  for (SmallPtrSet<GlobalValue*, 8>::iterator I = Used.begin(), E =
Used.end();
+      I != E; ++I)
+    CompilerUsed.erase(*I);

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.

+      if (Used.count(J)) {
+        Used.erase(J);
+        Used.insert(Target);
+      }

if (Used.erase(J)) {
  Used.insert(Target);
}

Same for CompilerUsed right after it.

Nick

On 6 May 2013 06:16, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

> >> I ended implementing two options. The first one (add-back.patch)
> >> looked the simplest at first: Just call replaceAllUsesWith and patch
> >> llvm.used back to its previous state. The main problem in here is
> >> detecting if any other changes were made. We have to walk the
> >> constant's uses to find out.
> >>
> >> Since we ale walking the uses anyway, another option (cow.patch) is to
> >> duplicate a bit of the copy on write logic in replaceAllUsesWith to
> >> special case use chains that end up in llvm.used. In the end I think I
> >> like this one a bit more. What do you think?
> >
> >
> > Yes, I also prefer cow.patch. Thanks for looking into this!
>
> Sorry, I noticed there was a third option:
> * Call replaceAllUsesWith only if there uses other than the ones from
> llvm.used and llvm.compiler_used.
> * Rebuild llvm.used and llvm.compiler_used only at the end of
> OptimizeGlobalAliases.
>
> This has the advantage that it is easy to extend to handle the extra
> optimization where we rename the target. With that enabled clang
> compiles
>
> extern "C" {
> __attribute__((used)) static void foo() {}
> }
>
> to
>
> @llvm.used = appending global [1 x i8*] [i8* bitcast (void ()* @foo to
> i8*)], section "llvm.metadata"
> define internal void @foo() #0 {
> entry:
>   ret void
> }
>
> Which is really cool. LLVM understands the hack clang did to keep the
> name foo visible and removes it :-)
>
> > Nick
>
> Cheers,
> Rafael
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130507/67a92578/attachment.html>