[PATCH] D23911: [SimplifyCFG] Change the algorithm in SinkThenElseCodeToEnd

[Hans Wennborg via llvm-commits]

hans added a comment.

Nice! But I had to struggle a bit to wrap my head around the code. I've left some questions hoping it's possible to make things clearer.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1322
@@ -1321,60 +1321,3 @@
 
-// Return true if V0 and V1 are equivalent. This handles the obvious cases
-// where V0 == V1 and V0 and V1 are both identical instructions, but also
-// handles loads and stores with identical operands.
-//
-// Because determining if two memory instructions are equivalent
-// depends on control flow, the \c At0 and \c At1 parameters specify a
-// location for the query. This function is essentially answering the
-// query "If V0 were moved to At0, and V1 were moved to At1, are V0 and V1
-// equivalent?". In practice this means checking that moving V0 to At0
-// doesn't cross any other memory instructions.
-static bool areValuesTriviallySame(Value *V0, BasicBlock::const_iterator At0,
-                                   Value *V1, BasicBlock::const_iterator At1) {
-  if (V0 == V1)
-    return true;
-  
-  // Also check for instructions that are identical but not pointer-identical.
-  // This can include load instructions that haven't been CSE'd.
-  if (!isa<Instruction>(V0) || !isa<Instruction>(V1))
-    return false;
-  const auto *I0 = cast<Instruction>(V0);
-  const auto *I1 = cast<Instruction>(V1);
-  if (!I0->isIdenticalToWhenDefined(I1))
-    return false;
-
-  if (!I0->mayReadOrWriteMemory())
-    return true;
-
-  // Instructions that may read or write memory have extra restrictions. We
-  // must ensure we don't treat %a and %b as equivalent in code such as:
-  //
-  //  %a = load %x
-  //  store %x, 1
-  //  if (%c) {
-  //    %b = load %x
-  //    %d = add %b, 1
-  //  } else {
-  //    %d = add %a, 1
-  //  }
-
-  // Be conservative. We don't want to search the entire CFG between def
-  // and use; if the def isn't in the same block as the use just bail.
-  if (I0->getParent() != At0->getParent() ||
-      I1->getParent() != At1->getParent())
-    return false;
-
-  // Again, be super conservative. Ideally we'd be able to query AliasAnalysis
-  // but we currently don't have that available.
-  auto WritesMemory = [](const Instruction &I) {
-    return I.mayReadOrWriteMemory();
-  };
-  if (std::any_of(std::next(I0->getIterator()), At0, WritesMemory))
-    return false;
-  if (std::any_of(std::next(I1->getIterator()), At1, WritesMemory))
-    return false;
-  return true;
-}
-
 // All blocks in Blocks unconditionally jump to a common successor. Analyze
 // the last non-terminator instruction in each block and return true if it would
----------------
There's no Blocks parameter anymore, so the comment needs updating.

And is it checking that all instructions in Insts can be sunk together? Maybe rename to canSinkInstructions?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1348
@@ -1412,2 +1347,3 @@
 
-  // If this isn't a store, check the only user is a single PHI.
+  // If this isn't a store, check the only user is a single PHI or in this
+  // block. If a user is in this block then it must come after I0 and
----------------
"this" is actually referring to all instructions in Instructions right? This keeps throwing me off, maybe it can be clarified somehow.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1368
@@ -1431,3 +1367,3 @@
     };
     if (!all_of(Insts, SameAsI0)) {
       if (I0->mustOperandBeConstant(OI))
----------------
do we know that all the instructions in Insts have the same number of operands as I0?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1488
@@ +1487,3 @@
+  // each block upwards in lockstep. If the n'th instruction from the end of each
+  // block can be sunk, the values are added to ValuesToSink and we carry on.
+  // If we can sink an instruction but need to PHI-merge some operands (because
----------------
Does "the values" here refer to those n'th-last instructions or something else?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1495
@@ +1494,3 @@
+  SmallVector<SmallVector<Value*,4>,4> ValuesNeedingPHIs;
+  SmallVector<Value*,4> ThisValuesNeedingPHIs;
+  while (PopulateInsts(ScanIdx) &&
----------------
Pretty tricky names here, especially ValuesNeedingPHIs and ThisValuesNeedingPHIs.

Would it help if instead of "values needing phis" these were referred to as PHI operands? And maybe store in a DenseMap from "value (instruction?) to sink" to "PHI operands"?

Anyway I wonder if this could all be made more clear somehow.


Repository:
  rL LLVM

https://reviews.llvm.org/D23911