[PATCH] D53942: IR Outliner Pass

[Jessica Paquette via Phabricator via llvm-commits]

paquette requested changes to this revision.
paquette added a comment.
This revision now requires changes to proceed.

Thanks for putting this up! I think this patch needs some work though.

I noticed there are lots of inconsistencies in commenting and code style here. I wrote some comments, but the issues seem to be prevalent throughout the code. I recommend looking through the LLVM Coding Standards (https://www.llvm.org/docs/CodingStandards.html) and revising the patch based off of that.

This patch is also extremely large, which makes it difficult to give a proper review. I suggest breaking it into several small patches, as per the LLVM Developer Policy. (https://llvm.org/docs/DeveloperPolicy.html#incremental-development)

I noticed that throughout this code, there are many shared structures with the MachineOutliner. For example, the instruction mapping stuff already exists in the MachineOutliner. Instead of rewriting all of these structures, I recommend making use of them and pulling them out into include/llvm/Transforms/Utils etc. This would reduce the number of changes to existing code necessary to add an IR outliner.

Also, with a patch like this, you need to attach some test-cases. I suggest looking through the LLVM Testing Guide. (https://llvm.org/docs/TestingGuide.html) I also recommend running the LLVM test suite and attaching some code size results versus a vanilla compiler.

As a final point, I think it'd be better to just call the pass "Outliner" rather than "CodeSizeOutliner", just for consistency with the other passes in LLVM.



================
Comment at: include/llvm/Transforms/IPO.h:223
 
+//===----------------------------------------------------------------------===//
+/// createCodeSizeOutlinerPass - This pass outlines congruent parts of
----------------
This shouldn't be here.


================
Comment at: include/llvm/Transforms/IPO.h:226
+/// functions.
+///
+ModulePass *createCodeSizeOutlinerPass();
----------------
Extra line isn't necessary.


================
Comment at: include/llvm/Transforms/IPO/CodeSizeOutliner.h:1
+//===-- CodeSizeOutliner.h - DCE unreachable internal functions
+//------------------===//
----------------
I think this needs to be updated.

E.g, "Pulls common instruction sequences into functions."


================
Comment at: include/llvm/Transforms/IPO/CodeSizeOutliner.h:2
+//===-- CodeSizeOutliner.h - DCE unreachable internal functions
+//------------------===//
+//
----------------
These should be on the same line as the header, not a separate line.


================
Comment at: include/llvm/Transforms/IPO/CodeSizeOutliner.h:12
+// This transform outlines congruent chains of instructions from the current
+//   module.
+//
----------------
Unnecessary indent.


================
Comment at: include/llvm/Transforms/IPO/CodeSizeOutliner.h:23
+namespace llvm {
+/// Pass to remove unused function declarations.
+class CodeSizeOutlinerPass : public PassInfoMixin<CodeSizeOutlinerPass> {
----------------
I don't think this comment is correct.


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:1
+//===- Outliner.h - A generic outlining utility interface around the Utils lib
+//------===//
----------------
This can just be "A generic outlining utility interface."


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:28
+/// \brief A potential candidate to be outlined.
+struct OutlineCandidate {
+  /// A unique ID for this outline candidate.
----------------
Can we change this to `Outlining` or `Outliner` for consistency?


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:41
+
+  /// Instruction size that this candidate shares with the next.
+  unsigned SharedSizeWithNext = 0;
----------------
What does "instruction size" mean here? Number of instructions? This could be more clear.


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:48
+  // Accessors.
+  using Iterator = std::vector<unsigned>::iterator;
+  using ConstIterator = std::vector<unsigned>::const_iterator;
----------------
This should be `iterator` for consistency with the rest of LLVM.


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:49
+  using Iterator = std::vector<unsigned>::iterator;
+  using ConstIterator = std::vector<unsigned>::const_iterator;
+  Iterator begin() { return Occurrences.begin(); }
----------------
This should be `const_iterator` for consistency with the rest of LLVM.


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:60
+  void invalidate() { Benefit = 0; }
+  // Get the candidate at index /p Idx.
+  unsigned getOccurrence(size_t Idx) const {
----------------
\p Idx

This shows up a few times, so you might want to search + replace for it.


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:77
+
+std::vector<OutlineCandidate> findSequentialOutliningCandidates(
+    function_ref<bool(ArrayRef<unsigned>, unsigned)> PrePruneFn,
----------------
Might be better to pass in the std::vector as an output parameter instead of returning it?


================
Comment at: include/llvm/Transforms/Utils/Outliner.h:112-115
+  std::vector<void *> InstrVec;
+
+  /// Map<Instruction, Index in InstrVec>
+  DenseMap<void *, unsigned> InstructionToIdxMap;
----------------
Why `void*`?

Maybe it would be better to templatise the mapper class on the type of instruction. (e.g `MachineInstr`, `Instruction`)?


================
Comment at: lib/Passes/PassBuilder.cpp:880
+  // Add the late outlining pass.
+  if(isOptimizingForSize(Level))
+    MPM.addPass(CodeSizeOutlinerPass());
----------------
Is this intended to always be enabled when optimizing for size?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:1
+//===-- CodeSizeOutliner.cpp - Propagate constants through calls -----===//
+//
----------------
This header doesn't seem right...?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:64
+struct Input {
+  Input(unsigned short InstrNo, unsigned short OpNo)
+      : InstrNo(InstrNo), OpNo(OpNo) {}
----------------
Why `short`?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:73
+
+/// \brief Contains the information necessary for condensing constant inputs.
+// Condense any constant int inputs to the outlined function.
----------------
You don't need \brief here


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:75
+// Condense any constant int inputs to the outlined function.
+// example:
+//  void outlinedFn(int a, int b);
----------------
These should be capitalized?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:80
+// We identify that in all occurrences that arg 2 is simply
+//  a difference of 1 from arg 1.
+// void outlinedFn(int a) {
----------------
Unnecessary indent


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:86
+struct ConstantCondenseInstance {
+  /// The base input.
+  Input BaseInput;
----------------
What is the base input?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:96
+  /// The cost of this instance.
+  unsigned Cost;
+
----------------
`Cost` is initialized everywhere else, why not here?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:105
+
+/// \brief Information necessary for outlining specific to IR outlining.
+struct AdditionalCandidateData {
----------------
We use autobrief, so you don't need to add \brief here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:137
+// Helper function for getting the size in bits of a type
+//  in a multiple of /p WidestRegister.
+unsigned getTypeSize(const DataLayout &DL, Type *Ty, unsigned WidestRegister) {
----------------
Unnecessary indent, fix \p


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:150
+
+// Helper function for getting the register usage of type /p Ty.
+unsigned getRegisterUsage(const DataLayout &DL, Type *Ty,
----------------
Why does this return 0? It would be good to explain what the expected return values are here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:153
+                          unsigned WidestRegister) {
+  unsigned ret = 0;
+  // getTypeSize calls getTypeSizeInBits which expects that
----------------
Variables should be capitalized. See:

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:156-158
+  if (Ty != nullptr && Ty->isSized())
+    ret = getTypeSize(DL, Ty, WidestRegister) / WidestRegister;
+  return ret;
----------------
You could probably just return `getTypeSize(DL, Ty, WidestRegister) / WidestRegister;` here, and then just return 0 otherwise.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:161
+
+// Helper to get the cost of a constant.
+unsigned getGlobalValueCost(const Constant *C) {
----------------
The cost model should be documented here. What is the cost measuring?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:163-166
+  if (isa<Function>(C))
+    return TargetTransformInfo::TCC_Free;
+  if (isa<GlobalValue>(C))
+    return TargetTransformInfo::TCC_Basic;
----------------
There should probably be a comment explaining these decisions.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:183-184
+  }
+  if (CE->isCast())
+    return getGlobalValueCost(CE->getOperand(0));
+  return TargetTransformInfo::TCC_Free;
----------------
I'd move this above `if (CE->getOpcode() == Instruction::GetElementPtr)`.

Then we could do `if (CE->getOpcode()  != Instruction::GetElementPtr)` instead and just return `TargetTransformInfo::TCC_Free`. Then the more complicated `GetElementPtr` case can be moved out of the if block. I think that would be a bit more readable.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:191-193
+  ///
+  ///  Instruction Information Utilities.
+  ///
----------------
Extra lines above + below aren't necessary.

Also, why is this a Doxygen comment?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:212-216
+#ifndef NDEBUG
+      Instruction *I = getInstr(InstrIdx);
+      DEBUG(dbgs() << "Instruction Cost : " << Info->Cost << " : " << *I
+                   << "\n");
+#endif
----------------
This can probably be under an `LLVM_DEBUG` macro?

e.g

```
LLVM_DEBUG(
  ... blah ...
);
```




================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:220
+  }
+  // Get the instruction info attached to index /p InstrIdx
+  InstructionInfo &getInstrInfo(size_t InstrIdx) {
----------------
InstructionInfo is a type, and so you should refer to it as such in the comment here.

Also, this isn't a Doxygen comment, so there's no reason to have a \p.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:232
+    Instruction *Inst = getInstr(InstrIdx);
+    BasicBlock *InstPar = Inst->getParent();
+
----------------
`ParentBB` or something similar is probably a bit more idiomatic.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:234
+
+    /// Inputs.
+    unsigned NumOperands = Inst->getNumOperands();
----------------
This doesn't need to be a Doxygen comment.

Also this could be more descriptive. What is it that you're calculating here?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:238
+    for (unsigned InIt = 0; InIt < NumOperands; ++InIt) {
+      unsigned IIdx = 0;
+      Value *Op = Inst->getOperand(InIt);
----------------
This could use a better name, or a comment explaining what it is.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:245-246
+          IIdx = FoundIIdx;
+      } else if (isa<BasicBlock>(Op))
+        break;
+      Info->InputIndexes.emplace_back(IIdx);
----------------
I'd move this to the top of the loop.

e.g
```
if (isa<BasicBlock>(Op))
    break;

Instruction *IOP = dyn_cast<Instruction>(Op);
```

...



================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:250
+
+    /// Outputs.
+    for (User *Usr : Inst->users()) {
----------------
This doesn't need to be a Doxygen comment.

It could be more descriptive as well. What is it that you're calculating?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:252
+    for (User *Usr : Inst->users()) {
+      Instruction *I = dyn_cast<Instruction>(Usr);
+      if (!I || I->getParent() != InstPar) {
----------------
This could use a better name. `UserInst` or something similar.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:263-265
+  ///
+  ///  Candidate Information Utilities.
+  ///
----------------
This probably shouldn't be a Doxygen comment.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:285
+private:
+  // Compute the cost of an instruction.
+  unsigned computeInstrCost(TargetTransformInfo &TTI, unsigned InstrIdx) {
----------------
Cost model should be documented.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:287
+  unsigned computeInstrCost(TargetTransformInfo &TTI, unsigned InstrIdx) {
+    Instruction *I = getInstr(InstrIdx);
+    const DataLayout &Layout = I->getModule()->getDataLayout();
----------------
In some places, you use `Inst` as the name of the `Instruction` you're looking at. In other places, you use `I`. It would be good to just pick one of them and be consistent to make it easier to follow the code.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:309
+    case Instruction::URem:
+      Cost = TargetTransformInfo::TCC_Basic * 2;
+      break;
----------------
Why times two?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:317
+    }
+    case Instruction::Load: {
+      LoadInst *LI = cast<LoadInst>(I);
----------------
This case is pretty complex. Since calls have their own helper function, I think that this should have one too for consistency. IMO, most of the cases in here probably should for the sake of consistency.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:323-324
+      // Be conservative about the cost of loads given they may be folded.
+      //  Estimating a lower cost helps to prevent over estimating the
+      //  benefit of this instruction.
+      Value *Ptr = LI->getPointerOperand();
----------------
Unnecessary indent.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:337
+        // Non Free address comp will likely need to load globals or values
+        //  used more than once.
+        return !isa<Constant>(Ptr) && I->hasOneUse();
----------------
Unnecessary indent, "Free probably shouldn't" be capitalized.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:352-353
+
+      // Be conservative about non free global geps that appear more than once
+      //  in a function, the gep is likely to be materialized only once.
+      GlobalVariable *GVar = dyn_cast<GlobalVariable>(GEP->getPointerOperand());
----------------
Why is that true?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:357
+        break;
+      if (HasFreeAddressComp) {
+        for (User *Usr : GVar->users()) {
----------------
Logic can probably be tightened up here. E.g.

```
if (!HasFreeAddressComp)
    break;

for (User *Usr : GVar->users()) {
...
}
break;
```


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:358-365
+        for (User *Usr : GVar->users()) {
+          Instruction *InstrUsr = dyn_cast<Instruction>(Usr);
+          if (!InstrUsr || InstrUsr == I)
+            continue;
+          if (InstrUsr->getFunction() != I->getFunction())
+            continue;
+          Cost = TargetTransformInfo::TCC_Free;
----------------
This could probably be simplified using `llvm::any_of`.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:377
+    // Accumulate additional cost from constant and global operands.
+    for (auto OpI = I->op_begin(), OpE = I->op_end(); OpI != OpE; ++OpI) {
+      Constant *COp = dyn_cast<Constant>(&*OpI);
----------------
Can this be moved above the switch statement? Then the switch could just add to `Cost` and return rather than breaking everywhere? (In the spirit of https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)

Also can `OpI` be a reference?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:394
+    }
+    return Cost;
+  }
----------------
This function is quite large, would it be possible to break it into some smaller pieces? E.g, the for loop after the switch could be a helper function etc.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:410
+    // Otherwise compute the register usage of the call instead of just
+    // a param count.
+    const DataLayout &Layout = CS.getParent()->getModule()->getDataLayout();
----------------
Please be consistent in usage of terminology; there are many comments that use "parameter", and many that use "param". I suggest using "parameter" always, since that seems more idiomatic.

See: https://llvm.org/docs/CodingStandards.html#commenting


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:415-418
+    for (Value *Op : CS.args()) {
+      TotalParamSizeInBits +=
+          getTypeSize(Layout, Op->getType(), WidestRegister);
+    }
----------------
This loop doesn't need braces.




================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:424
+    }
+    // Call instruction.
+    unsigned CallCost = TargetTransformInfo::TCC_Basic;
----------------
This comment isn't useful. It would be better to describe what you're going to do with this in detail.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:434
+
+  /// Stores information for parallel instruction in InstrVec.
+  std::vector<InstructionInfo *> InstrInfo;
----------------
s/instruction/instructions/

Also, why parallel?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:443
+
+/// \brief A specific instance of an outlined candidate.
+struct FunctionSplicer {
----------------
Outlined? Or outlining?

Also, this name seems strange for what this thing is?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:474-486
+    if (NumOutputs == 0)
+      OutputType = Type::getVoidTy(Ctx);
+    else if (NumOutputs == 1) {
+      Instruction *OnlyOut =
+          OM.getInstr(InitialStartIdx + Data.Outputs.find_first());
+      OutputType = OnlyOut->getType();
+    } else {
----------------
I think that this is one of those cases where you can use braces around an if. :)


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:493
+
+  // Outline a new occurrence of an outline chain.
+  void outlineOccurrence(const OutlineCandidate &OC, unsigned StartIdx,
----------------
Please define terminology you'll use throughout the code. There is no definition of "outline chain" anywhere.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:499
+    Function *ParentFn = Tail->getFunction();
+    bool InitialOccur = !OutlinedFn;
+    AdditionalCandidateData &Data = OM.getCandidateData(OC);
----------------
It would be nice to have an explanation of why variables in the code are important.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:502
+
+    /// Split the outline chain into its own block.
+    Instruction *Head = OM.getInstr(StartIdx);
----------------
This shouldn't be a Doxygen comment.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:506
+    // Split our chain instance into a separate block for extraction.
+    /// Split up to the head if we aren't at the front of our block.
+    if (Head != &EntryBlock->front() ||
----------------
No doxygen here either.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:510
+      EntryBlock = EntryBlock->splitBasicBlock(Head->getIterator());
+    /// Split after the tail.
+    BasicBlock *Exit = nullptr;
----------------
No doxygen.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:513
+    if (!Tail->isTerminator()) {
+      auto SentinalIt = Tail->getNextNode()->getIterator();
+      Exit = EntryBlock->splitBasicBlock(SentinalIt);
----------------
s/Sentinal/Sentinel/


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:521
+
+    // Create parameter vec for the new call.
+    unsigned NumOutputs = Data.Outputs.count();
----------------
Because comments should be written like English prose, please use "vector" rather than "vec".


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:533
+
+    // Get the first valid debug info.
+    DebugLoc CallLoc;
----------------
I feel like this needs better explanation in general. Comments like these are fine, but the overall gist of the function needs to be written down somewhere.

For example,

"This function works by doing (blah). First we (do thing), then we (do other thing), and finally we (do another thing). At the end of this, (something) will (be in state)."


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:540-541
+
+    // If this is not the first occurrence we merge metadata and attributes for
+    //  the outlined instructions.
+    if (!InitialOccur) {
----------------
- This needs explanation.
- Extra indent.
- I recommend splitting this into two comments. The first can be something like, "Is this the first occurrence?", and the second can be something like "No. Merge metadata and attributes for the outlined instructions."


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:555
+
+      // Merge special state.
+      for (unsigned InitI = InitialStartIdx, CurI = StartIdx,
----------------
What is special state?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:561-562
+        Instruction *CurII = OM.getInstr(CurI);
+        // Make sure the alignment is valid as we skip it during congruency
+        // finding.
+        if (LoadInst *LI = dyn_cast<LoadInst>(InitII))
----------------
This sentence is awkward. The phrase "congruency finding" is kind of clunky IMO.

Also, once again, please define terminology you'll use throughout the code. I haven't seen a definition of "congruency" anywhere. That's a pretty heavily-overloaded math word, so it's good to make sure everyone is on the same page early in.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:569
+                                    cast<StoreInst>(CurII)->getAlignment()));
+        // Make sure that no tails are propagated properly.
+        else if (CallInst *CI = dyn_cast<CallInst>(InitII)) {
----------------
No tails? Is that correct?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:576-577
+        // Be conservative about flags like nsw/nuw.
+        else
+          InitII->andIRFlags(OM.getInstr(CurI));
+
----------------
Please be consistent with braces.

Also, when I see something like this, it smells like the logic can probably be tightened up a bit.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:591
+      FunctionType *FTy = OutlinedFn->getFunctionType();
+      for (unsigned i = 0, e = Args.size(); i < e; ++i) {
+        Value *&Arg = Args[i];
----------------
I'm seeing a lot of inconsistencies in the variable names in for loops here.

Also, can this just be a range-based for?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:599
+
+      // Otherwise we outline the initial occurrence.
+    } else
----------------
This should be in the else case.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:600-601
+      // Otherwise we outline the initial occurrence.
+    } else
+      outlineInitialOccurrence(OC, OM, StartIdx, Args, EntryBlock);
+
----------------
Once again, consistency with braces?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:603
+
+    // Create the patchup for the outline section.
+    Instruction *PatchupI;
----------------
I think basically everywhere there's a comment like this should have its own helper function. This function is very long.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:613
+      PatchupI = NewII;
+      // Otherwise create a call instruction.
+    } else {
----------------
This comment should be in the else case.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:625
+      OnlyOut->replaceUsesOutsideBlock(PatchupI, EntryBlock);
+    } else if (NumOutputs != 0) {
+      unsigned OutputNum = 0;
----------------
`NumOutputs > 0` makes the assumption here more clear.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:644
+
+  // \brief Finalize this function as an outline instance.
+  void finalize(const OutlineCandidate &OC, IROutlinerMapper &OM) {
----------------
What does finalize mean here?

Also, \brief is meaningless outside of Doxygen comments. This should be like

```
/// Finalize ...
```


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:648-660
+    Instruction *Tail = OM.getInstr(InitialStartIdx + OC.Len - 1);
+    if (InvokeInst *II = dyn_cast<InvokeInst>(Tail)) {
+      SmallVector<Value *, 8> IIArgs(II->arg_operands());
+      SmallVector<OperandBundleDef, 2> Bundles;
+      II->getOperandBundlesAsDefs(Bundles);
+      CallInst *CI =
+          CallInst::Create(II->getCalledValue(), IIArgs, Bundles, "", II);
----------------
This probably deserves its own function.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:679
+private:
+  // Re-Unique the inputs for the outlined function.
+  //  example:
----------------
Re-unique


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:685
+  // We identify that parameters 1 and 2 are the same
+  //  for each callsite, so we condense them.
+  void uniqueInputs(AdditionalCandidateData &Data) {
----------------
Extra indent.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:690
+
+    // Map <ArgNo> to <Congruency Group Idx>
+    DenseMap<unsigned, unsigned> ArgNoToCG, RhsArgNoToCG;
----------------
"Group" is super confusing when you're using the word "congruency" here.

Some people might end up thinking of things like this when the terminology isn't defined:
https://en.wikipedia.org/wiki/Congruence_subgroup


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:702
+      for (unsigned i = 0, e = NumInputs; i < e; ++i) {
+        // We already evaluated the equivalencies for this arg.
+        if (ANoToCG.count(i))
----------------
s/arg/argument/

Also, it makes more sense to say what you're *going to do*, since we're reading top-to-bottom. For example,

"Did we already evaluate the equivalencies for this argument? If so, then skip it."


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:721
+
+    // Build initial equivalences from the first call.
+    auto UserIt = OutlinedFn->user_begin();
----------------
This smells like it deserves its own helper function.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:728
+    // If we have the same amount of congruency groups as we do arguments,
+    //   the they are already unique.
+    if (NumInputs == ArgCongruencyGroups.size())
----------------
then


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:733
+    BitVector ResolvedInputs(NumInputs);
+    // BitVector helper to hold non congruent matches between argument groups.
+    BitVector NonCongruentLeft;
----------------
non-congruent


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:747
+
+        /// Build non congruent arguments between the groups.
+        NonCongruentLeft = LhsGroup;
----------------
"non-congruent"

This shows up in a few places, so I guess a search + replace should help here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:751
+        LhsGroup &= RhsGroup;
+        assert(LhsGroup.count() > 0);
+
----------------
This could use a string explaining why you want this to be true.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:763
+        // Move the non congruent matches from the left group to a
+        //   new congruency group.
+        unsigned NewGroupId = ArgCongruencyGroups.size();
----------------
I'm not sure if clang-format will catch these, but... it would be good to remove all of these extra indents.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:778
+
+    // Build new function from the condensed inputs.
+    std::vector<Type *> NewFnTys;
----------------
s/Build new/Build a new/

Also, it's weird that this function is doing so many things. It should probably be split up into different tasks.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:791
+
+    /// Move Fn Body.
+    MergedFn->getBasicBlockList().splice(MergedFn->begin(),
----------------
  - Shouldn't be a Doxygen comment.
  - s/Fn/function/
  - Body doesn't need to be capitalized.




================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:809
+      CallArgs.clear();
+      /// Map call args by their congruency group.
+      for (auto &It : ArgCongruencyGroups)
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:812
+        CallArgs.push_back(CS.getArgOperand(It.find_first()));
+      /// Create the new call.
+      Instruction *NewI;
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:826
+      } else
+        llvm_unreachable("Invalid callsite for outlined function.");
+      NewI->setDebugLoc(OldI->getDebugLoc());
----------------
Consistency with braces?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:831-832
+    }
+    OutlinedFn->eraseFromParent();
+    OutlinedFn = MergedFn;
+  }
----------------
There should be a comment here explaining what you did at the end of the function, and what's true now.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:835
+
+  // \brief Outline the initial occurrence of this chain.
+  void outlineInitialOccurrence(const OutlineCandidate &OC,
----------------
Replace with a doxygen comment.

```
/// Outline...
```

Also why is this a separate case?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:841
+
+    /// Function type for outlined function.
+    SmallVector<Type *, 8> Tys;
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:848
+
+    /// Create function and move outlined block.
+    OutlinedFn = Function::Create(FTy, GlobalValue::PrivateLinkage, "",
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:851
+                                  Entry->getModule());
+    OutlinedFn->setCallingConv(CallingConv::Fast);
+    OutlinedFn->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
----------------
This needs a comment explaining why (along with the other attributes).


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:858
+
+    /// Inherit attributes.
+    LLVMContext &Ctx = OutlinedFn->getContext();
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:866
+
+    // Drop any uses of the outlined instructions as metadata.
+    for (unsigned InitI = StartIdx, InitE = StartIdx + OC.Len; InitI < InitE;
----------------
Is this a safe thing to do? Can you explain why in the comment?


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:875
+    // FIXME: Ideally we should compute the real count for this function but
+    //  for now we just tag it as cold.
+    if (EmitProfileData)
----------------
Is this something you should really do? I can understand the motivation, but I don't know if I agree with tagging things as cold when they really might not be.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:886
+
+    /// Create stores for any output variables.
+    Value *RetVal = nullptr;
----------------
Don't use Doxygen here.

Also this needs its own function. This function is too long.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:891
+      RetVal = OM.getInstr(StartIdx + Data.Outputs.find_first());
+    else if (NumOutputs != 0) {
+      RetVal = UndefValue::get(OutputType);
----------------
`NumOutputs > 0`?

I feel like this could be tightened up a bit more as is anyway...


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:904
+
+    /// Replace input operands with function arguments.
+    auto ArgI = OutlinedFn->arg_begin();
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:911
+
+    /// Insert the constant param fixups.
+    for (ConstantCondenseInstance &CInst : Data.ConstInsts) {
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:931
+
+    /// Cleanup for ignored instructions.
+    for (auto It = Entry->begin(), E = Entry->end(); It != E;) {
----------------
Don't use Doxygen here.


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:955
+  Type *OutputType = nullptr;
+  /// If we are emitting profile date during outlining.
+  bool EmitProfileData;
----------------
"Set if" or "True if"


================
Comment at: lib/Transforms/IPO/CodeSizeOutliner.cpp:965
+
+/// \brief Perform analysis and verification for the found outline candidates.
+struct OutlinerAnalysis {
----------------
Don't need \brief.

"Outline" is clunky, probably better to use "outlining".


Repository:
  rL LLVM

https://reviews.llvm.org/D53942