[PATCH] D106995: [IROutliner] Allowing PHINodes in Exit Blocks
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 15 13:02:16 PDT 2021
paquette added inline comments.
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:884
+ // to keep it as an output.
+ for (unsigned Idx = 0, Edx = PN.getNumIncomingValues(); Idx < Edx; Idx++) {
+ if (Idx == PHILoc)
----------------
don't recalculate loop invariant
also can this just be an `any_of`?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:895
+ // We examine the other uses of the value to see if it used outside the
+ // region.
+ for (User *U : V->users()) {
----------------
this comment could be more succinct, like "Check if the value is used by any instructions outside of the region."
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:896
+ // region.
+ for (User *U : V->users()) {
+ Instruction *I = dyn_cast<Instruction>(U);
----------------
this could probably also be an `any_of`
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:903
+ // inside the region doesn't tell us anything about the uses outside
+ // non exit blocks outside the region.
+ BasicBlock *Parent = I->getParent();
----------------
This sentence is pretty long and kind of confusing. Can you simplify it?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:908
+
+ // If it's not a PHINode then we definitely know the use matters.
+ if (!isa<PHINode>(I))
----------------
can you explain this a bit more?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:923
+
+/// We check the exit block, and test whether it has any PHINodes that should
+/// be considered outputs as it combines a value used as an output in the
----------------
this is kind of wordy, maybe rewrite it like
"Test whether \p ExitBlock contains any PhiNodes that should be considered outputs. PhiNodes should be outputs when...(blah)"
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:928
+/// \param ExitBB [in] - The block to analyze.
+/// \param ExitBBs [in] - The potential exit blocks from the region.
+/// \param BBSet [in] - The basic blocks in the region.
----------------
the comments here don't line up with the variable names in the function
this could be a more descriptive name as well, like "PotentialExitsFromRegion"
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:929
+/// \param ExitBBs [in] - The potential exit blocks from the region.
+/// \param BBSet [in] - The basic blocks in the region.
+/// \param Outputs [in, out] - The existing outputs for the region, we may add
----------------
this could be named "BasicBlocksInRegion"
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:932
+/// PHINodes to this as we find that they replace output values.
+/// \param PHIWrapped [out] - A set containing outputs that are totally replaced
+/// by a PHINode.
----------------
I think this variable would be better-named "OutputsReplacedByPhis"
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:934
+/// by a PHINode.
+/// \param NotPHIWrapped [out] - A set containing outputs that are used in
+/// PHINodes, but have other uses, and should still be considered outputs.
----------------
maybe this could have a more descriptive name too, like "OutputsWithNonPhiUses" or "OutputsThatCouldNotBeReplacedWithPhis"
(second option is kind of wordy, but it describes what's happening pretty well)
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:945
+ SmallVector<unsigned, 2> IncomingVals;
+ for (unsigned I = 0, E = PN.getNumIncomingValues(); I < E; ++I)
+ if (BBSet.contains(PN.getIncomingBlock(I)))
----------------
don't recalculate loop invariant
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1003
+
+/// We need to create a special GVN for PHINodes that will be used outside of
+/// the region. We create a hash code based on the Canonical number of the
----------------
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1012
+/// \param Region - The region that \p PN is an output for.
+/// \param BBSet - The BasicBlocks containsed in the region.
+/// \param PN - The PHINode we are analyzing.
----------------
this could probably be called "BasicBlocksInRegion" or something
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1021
+ OutlinableGroup &Group = *Region.Parent;
+ IRSimilarityCandidate &C = *Region.Candidate;
+ BasicBlock *PHIBB = PN->getParent();
----------------
can this be named `Cand` or something?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1024
+ CanonList PHIGVNs;
+ for (unsigned Idx = 0, EIdx = PN->getNumIncomingValues(); Idx < EIdx; ++Idx) {
+ Value *V = PN->getIncomingValue(Idx);
----------------
since you don't use the index here, could this just be a loop like this?
```
for (Value *Incoming : PN->incoming_values()) {
...
}
```
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1027
+ // If we cannot find a GVN, this means that the input to the PHINode is
+ // not included in the region we are trying to analyze, meaning, that if
+ // it was outlined, we would be adding an extra input. We ignore this
----------------
comment is pretty wordy, split it up?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1030
+ // case for now, and so ignore the region.
+ if (!C.getGVN(V).hasValue()) {
+ Region.IgnoreRegion = true;
----------------
can you pull `C.getGVN(V)` into a variable, since you reuse it below?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1037
+ unsigned GVN = C.getGVN(V).getValue();
+ GVN = *C.getCanonicalNum(GVN);
+ PHIGVNs.push_back(GVN);
----------------
is this guaranteed to have a value?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1038
+ GVN = *C.getCanonicalNum(GVN);
+ PHIGVNs.push_back(GVN);
+ }
----------------
simpler?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1040
+ }
+ DenseMap<hash_code, unsigned>::iterator GVNToPHIIt;
+ DenseMap<unsigned, PHINodeData>::iterator PHIToGVNIt;
----------------
can you add a comment explaining what you're going to do in this portion of the function here?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1094
// the PHINodes for whether they should no longer be considered outputs.
- for (BasicBlock *ExitBB : Exits) {
- for (PHINode &PN : ExitBB->phis()) {
- // Find all incoming values from the outlining region.
- SmallVector<unsigned, 2> IncomingVals;
- for (unsigned Idx = 0; Idx < PN.getNumIncomingValues(); ++Idx)
- if (BBSet.contains(PN.getIncomingBlock(Idx)))
- IncomingVals.push_back(Idx);
-
- // Do not process PHI if there is one (or fewer) predecessor from region.
- if (IncomingVals.size() <= 1)
- continue;
-
- Region.IgnoreRegion = true;
- return;
- }
- }
+ DenseSet<Value *> PHIWrapped;
+ DenseSet<Value *> NotPHIWrapped;
----------------
these could use more descriptive names
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1153
std::make_pair(Group.ArgumentTypes.size() - 1, OriginalIndex));
- Region.GVNStores.push_back(GlobalValue);
+ AggArgIdx = Group.ArgumentTypes.size() - 1;
}
----------------
pull `Group.ArgumentTypes.size()` into a variable and reuse it? it's calculated 4 times in this function.
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1162
+ // Since values outside the region can be combined into PHINode when we
+ // have multiple exists, we collect both of these into a list to identify
+ // which values are being used in the PHINode. Each list identifies a
----------------
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1164
+ // which values are being used in the PHINode. Each list identifies a
+ // different PHINode, and a different output, so we store it as it's own
+ // canonical value. These canonical values are also dependent on the
----------------
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1166
+ // canonical value. These canonical values are also dependent on the
+ // output argument it is saved to.
+
----------------
this comment has a lot of run-ons; can you split it up a little?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1168
+
+ // If a PHINode as the same canonical values, but a different aggregate
+ // argument location, they will have distinct Canonical Values, to be used
----------------
this comment is confusing. Is it two PhiNodes or one?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1170
+ // argument location, they will have distinct Canonical Values, to be used
+ // for the store sets.
+ GVN = getGVNForPHINode(Region, BBSet, PN, AggArgIdx);
----------------
can you expand on the store sets a little more? I feel like it's kind of out of nowhere.
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1322
+/// Attempt to find a block containing PHIBlocks for the given \p RetVal, and
+/// create one to add to the overall function if it does not exist.
----------------
simpler:
Find or create a BasicBlock in the outlined function containing PhiBlocks for \p RetVal.
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1333
+ DenseMap<Value *, BasicBlock *>::iterator PHIVBBIt, RetBBIt;
+ PHIVBBIt = Group.PHIBlocks.find(RetVal);
+ RetBBIt = Group.EndBBs.find(RetVal);
----------------
I think this could be called like "PhiBlockForRetVal"
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1334
+ PHIVBBIt = Group.PHIBlocks.find(RetVal);
+ RetBBIt = Group.EndBBs.find(RetVal);
+ assert(RetBBIt != Group.EndBBs.end() && "Could not find output value!");
----------------
this could also use a more descriptive name
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1358
+ Instruction *Term = Pred->getTerminator();
+ BranchInst *BI = static_cast<BranchInst *>(Term);
+ BranchesToChange.push_back(BI);
----------------
- this can be a one-liner
- I think this should use `cast`?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1362
+ for (BranchInst *BI : BranchesToChange) {
+ for (unsigned Succ = 0; Succ < BI->getNumSuccessors(); Succ++) {
+ if (BI->getSuccessor(Succ) != ReturnBB)
----------------
don't recalculate loop invariant
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1374
+
+/// For the function call now representing the \p Region, find the passed value
+/// to that call that represents Argument \p A at the call location.
----------------
simpler:
\returns The Value representing \p A at the call location for some outlined function.
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1378
+/// \param A - The Argument to get the passed value for.
+/// \param Region - The extracted Region to get the passed Argument to.
+/// \param AdjustArgNo - The boolean value, if true, we must adjust the
----------------
maybe this should be "the region corresponding to the outlined function" or something?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1379
+/// \param Region - The extracted Region to get the passed Argument to.
+/// \param AdjustArgNo - The boolean value, if true, we must adjust the
+/// argument location as we are not analyzing the overall function call, but
----------------
or even simpler:
"True when \p A's location must be adjusted. This happens when... (stuff)."
I think this could also be named something like "HasAlreadyBeenReplacedWithOutlinedCall" or something?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1381
+/// argument location as we are not analyzing the overall function call, but
+/// extracted functionn call. If false, we do not have to make this adjustment.
+/// \returns The passed value to the function now representing the region for
----------------
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1414
+/// \p PN.
+/// \param AdjustArgNo - A flag to use the extracted function call of \p Region
+/// rather than the overall function's call.
----------------
this could use a better name (like above)
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1417
+static void findCanonNumsForPHI(
+ PHINode * PN, OutlinableRegion & Region,
+ const DenseMap<Value *, Value *> &OutputMappings,
----------------
formatting
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1434
+ assert(GVN.hasValue() && "No GVN for incoming value");
+ Optional<unsigned> CanonNum = Region.Candidate->getCanonicalNum(*GVN);
+ CanonNums.insert(*CanonNum);
----------------
is this guaranteed to have a value?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1452
+findOrCreatePHIInBlock(PHINode *PN, OutlinableRegion &Region,
+ BasicBlock *OverallPhiBlock,
+ const DenseMap<Value *, Value *> &OutputMappings) {
----------------
should this ever be null? can it be a reference?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1459
+ // the overall function yet. We make sure to reassign the argument numbering
+ // since it is possible that
+ findCanonNumsForPHI(PN, Region, OutputMappings, PNCanonNums, true);
----------------
unfinished comment?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1462
+
+ OutlinableRegion *First = Group.Regions[0];
+ DenseSet<unsigned> CurrentCanonNums;
----------------
this could use a more descriptive name or a comment
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1478
+ // we must insert it ourselves.
+ PHINode *NewPN = static_cast<PHINode *>(PN->clone());
+ NewPN->insertBefore(&*OverallPhiBlock->begin());
----------------
```
PHINode *cloneImpl() const;
```
do you have to cast here? `static_cast` seems wrong too
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1482
+ Idx++) {
+ Value *IVal = NewPN->getIncomingValue(Idx);
+ BasicBlock *IBlock = NewPN->getIncomingBlock(Idx);
----------------
just call this IncomingVal?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1483
+ Value *IVal = NewPN->getIncomingValue(Idx);
+ BasicBlock *IBlock = NewPN->getIncomingBlock(Idx);
+
----------------
just call this IncomingBlock?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1487
+ // block.
+ Instruction *Inst = IBlock->getFirstNonPHI();
+ assert(Inst && "Incoming block is empty?");
----------------
call this FirstNonPhi?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1489
+ assert(Inst && "Incoming block is empty?");
+ Value * TempVal = Region.findCorrespondingValueIn(*First, Inst);
+ assert(TempVal && "Value is nullptr?");
----------------
this could use a more descriptive name?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:2205
+/// For the \p OutputCanon number passed in find the value represented by this,
+/// canonical number. If it is from a PHINode, we pick one the first incoming
----------------
this sentence is confusing
```
\returns The Value represented by a canonical number \p OutputCanon in \p Region.
```
I assume this is an output as well? Should this function be called "findOutputValueInRegion" or something?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106995/new/
https://reviews.llvm.org/D106995
More information about the llvm-commits
mailing list