[PATCH] D21255: Fix cloning GlobalValues with comdats across Modules.

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/IR/Globals.cpp:103-110
@@ -100,1 +102,10 @@
+    // module as Src, so it's safe to share Src's comdat.
+    Comdat *SrcComdat = const_cast<GlobalObject *>(GV)->getComdat();
+    if (SrcComdat == nullptr || getParent() == nullptr)
+      setComdat(SrcComdat);
+    else if (getParent() != nullptr) {
+      Comdat *C = getParent()->getOrInsertComdat(SrcComdat->getName());
+      C->setSelectionKind(SrcComdat->getSelectionKind());
+      setComdat(C);
+    }
   }
----------------
majnemer wrote:
> I find it a little weird that we are trying to predict how the global will be used.
> We are not sure which module it will be in but we sorta assume it will be in the same one as the original function...
> 
> I'm starting to think that the right thing to do is move this logic out of copyAttributesFrom and into another method.  Something like copyComdatFrom which asserts that the source and dest are from the same module.
> 
> What do you think?
> I find it a little weird that we are trying to predict how the global will be used.

Me too.

An alternative would be not to set the comdat at all if the function isn't in a module.  That's how I originally had it, but it broke a test (which was what made me realize it was a functional change in the first place).

> Something like copyComdatFrom which asserts that the source and dest are from the same module.

...or maybe it would assert that just that src and dst are both in a module, and then copy the comdat as we do here?  That seems like basically what one would want, I think.

The problem with adding either of these new functions is that we'd have to audit the ~30 calls to copyAttributesFrom, and I'm not sure I'm going to be successful at that.


http://reviews.llvm.org/D21255