[PATCH] D106989: [IRSim] Finding Branch Similarity
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 26 15:20:56 PDT 2021
paquette added inline comments.
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:151
+ /// A function that for an IRInstructionData containing a branch, finds the
+ /// relative distances from the source basic block to the target by taking
----------------
remove fluff
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:305
+ /// A mapping for a basic block in a module to it's assigned number/location
+ /// in the module.
----------------
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:130
+ SmallVector<int, 4> RelativeBlockLocations;
+
----------------
Why int?
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:359
+ /// Assigns values to all the basic blocks in Module \p M.
+ /// \param M - The modules containing the basic blocks to assign numbers to.
+ void initializeForBBs(Module &M) {
----------------
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:461
+
+ bool EnableBranches = false;
};
----------------
comment?
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:461
+
+ bool EnableBranches = false;
};
----------------
paquette wrote:
> comment?
comment on this variable?
================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:860
+ bool EnableBranches = true;
+
----------------
doxygen comment on this variable?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:26
+cl::opt<bool> DoNotEnableBranches(
+ "no-ir-sim-branch-matching", cl::init(false), cl::ReallyHidden,
----------------
Mention this is for debugging?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:175
+ if (isa<BranchInst>(A.Inst) && isa<BranchInst>(B.Inst)) {
+ if (A.RelativeBlockLocations.size() != B.RelativeBlockLocations.size())
+ return false;
----------------
pull `if` into the `if` above?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:687
+ // analagous location in the region we are comparing to.
+ if ((isa<BranchInst>(ItA->Inst) && isa<BranchInst>(ItB->Inst)) ||
+ (isa<PHINode>(ItA->Inst) && isa<PHINode>(ItB->Inst))) {
----------------
Cache `ItA->Inst` and `ItB->Inst` in variables?
also can you early return true or false using this condition? This `if` is pretty big.
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:702
+ OperValsA, OperValsB);
+ if (!all_of(ZippedRelativeLocations,
+ [&A, &B](std::tuple<int, int, Value *, Value *> R) {
----------------
this is a pretty dense statement. can you simplify this somehow? maybe it would be better as a helper function or something?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:712
+ B.getBasicBlocks(BasicBlockB);
+ if (BasicBlockA.find(ABB) != BasicBlockA.end())
+ AContained = true;
----------------
this could assign to AContained right?
```
bool AContained = BasicBloc A.find(...) != ...;
```
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:805
+
+ if (StringLen == 2) {
+ const unsigned StartIdx = *RS.StartIndices.begin();
----------------
why is there a special case for 2? comment?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:174
+ if (isa<BranchInst>(A.Inst) && isa<BranchInst>(B.Inst)) {
+ if (A.RelativeBlockLocations.size() != B.RelativeBlockLocations.size())
----------------
pull second `if` into this one?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:688
+ // analagous location in the region we are comparing to.
+ if ((isa<BranchInst>(ItA->Inst) && isa<BranchInst>(ItB->Inst)) ||
+ (isa<PHINode>(ItA->Inst) && isa<PHINode>(ItB->Inst))) {
----------------
de morgan this and early return true to reduce indentation?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1392
+
+ if (!IDIt.getNodePtr()->isKnownSentinel())
+ if (IDIt->Inst != NextInst)
----------------
I think that you should check for the end of the list in a different way. This seems like a workaround for a different bug? Nowhere else in the codebase uses `isKnownSentinel` directly.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106989/new/
https://reviews.llvm.org/D106989
More information about the llvm-commits
mailing list