[PATCH] D86978: [IROutliner] Deduplicating functions that only require inputs.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added a comment.

There's quite a few dead variables added in this patch. Do they belong in later patches in the series?



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:281
+  Group.OutlinedFunction->addFnAttr(Attribute::OptimizeForSize);
+  Group.OutlinedFunction->addFnAttr(Attribute::MinSize);
+
----------------
non-rhetorical question: should it be marked noduplicate too?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:294
+  BasicBlock *NewEnd = nullptr;
+  std::vector<Instruction *> DebugInsts;
+  for (CurrBB = Old.begin(), FinalBB = Old.end(); CurrBB != FinalBB;
----------------
dead variable


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:405
+findExtractedInputToOverallInputMapping(OutlinableRegion &Region,
+                                        std::vector<unsigned> InputGVNs,
+                                        SetVector<Value *> &ArgInputs) {
----------------
ArrayRef


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:408
+
+  IRSimilarityCandidate &C = *Region.Candidate;
+  OutlinableGroup &Group = *Region.Parent;
----------------
elsewhere you have usually called them `IRSC`


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:420-422
+    Optional<Value *> InputOpt = C.fromGVN(InputVal);
+    assert(InputOpt.hasValue() && "Global value number not found?");
+    Value *Input = InputOpt.getValue();
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:430
+    // mapping of the index to the value.
+    unsigned Found = ArgInputs.count(Input);
+    assert(Found && "Input cannot be found!");
----------------
I think this needs to be folded into the assertion to avoid -Wundef in Release builds.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:434
+    Region.ExtractedArgToAgg.insert(std::make_pair(OriginalIndex, TypeIndex));
+    Region.AggArgToExtracted.insert(std::make_pair(TypeIndex, OriginalIndex));
+    OriginalIndex++;
----------------
emplace


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:476
+  std::vector<Value *> NewCallArgs;
+  DenseMap<unsigned, unsigned>::iterator ArgPair;
+
----------------
both of these variables are dead


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:542-544
+  for (Attribute A :
+       CurrentOS->ExtractedFunction->getAttributes().getFnAttributes())
+    CurrentGroup.OutlinedFunction->addFnAttr(A);
----------------
Unless you need to filter them for some reason?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:561
+
+  std::vector<BasicBlock *> OutputStoreBBs;
+
----------------
variable is dead


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:563
+
+  OutlinableRegion *CurrentOS;
+
----------------
I'd sink this to where it's used.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:568
+  // Do the same for the other extracted functions
+  for (unsigned Idx = 1; Idx < CurrentGroup.Regions.size(); Idx++) {
+    CurrentOS = CurrentGroup.Regions[Idx];
----------------
Why is the first one skipped, and can this be a range-for?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86978/new/

https://reviews.llvm.org/D86978