[PATCH] D18100: Simplify Logic in IRMover

[Mehdi AMINI via llvm-commits]

joker.eph 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);
----------------
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.


http://reviews.llvm.org/D18100