[PATCH] D18100: Simplify Logic in IRMover

[Tobias Edler von Koch via llvm-commits]

tobiasvk added inline comments.

================
Comment at: lib/Linker/IRMover.cpp:1058
@@ -1058,11 +1057,3 @@
 
-  // just missing from map
-  if (ShouldLink) {
-    auto I = ValueMap.find(SGV);
-    if (I != ValueMap.end())
-      return cast<Constant>(I->second);
-
-    I = AliasValueMap.find(SGV);
-    if (I != AliasValueMap.end())
-      return cast<Constant>(I->second);
-  }
+  /// First verify if the value hasn't been alreay linked.
+  auto I = ValueMap.find(SGV);
----------------
joker.eph wrote:
> tobiasvk wrote:
> > Why can you drop "if (ShouldLink)"? That seems unrelated to merging the two maps.
> I'm not sure, can I ask the opposite question: "why not?"? ;) (this code is not documented so I have no clue...)
> 
> My impression is that if there is already a value in the map, then we should just use it. Why would linkGlobalValueProto() create a new one?
> Maybe it is an invariant of the method that we can't have !ShouldLink and the value being in the map. In this case the test if(ShouldLink) is just here to save querying the map. That may be my best bet, so I'll add the check back, but an assert in the else branch.
To be honest, I have no clue either ;)

Adding an assert sounds like a good option - at least it won't silently break if this is in fact needed.


http://reviews.llvm.org/D18100