[PATCH] D39352: [SimplifyCFG] Don't do if-conversion if there is a long dependence chain

[Mandeep Singh Grang via Phabricator via llvm-commits]

mgrang added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:405
+static int CountBBCodeSize(BasicBlock *BB, const TargetTransformInfo &TTI) {
+  int size = 0;
+  for (auto II = BB->begin(); !isa<TerminatorInst>(II); ++II)
----------------
Use uppercase variable names.

Shouldn't size be unsigned?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:413
+/// dependence chain containing the compare instruction.
+static int FindDependenceChainLatency(BasicBlock *BB,
+                            std::map<Instruction *, int> &instructions,
----------------
Shouldn't latency be unsigned?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:414
+static int FindDependenceChainLatency(BasicBlock *BB,
+                            std::map<Instruction *, int> &instructions,
+                            const TargetTransformInfo &TTI,
----------------
Uppercase variable names.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:417
+                            bool LongestChain) {
+  int max_latency = 0;
+
----------------
Uppercase variable names.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:423
+    for (unsigned O = 0, E = II->getNumOperands(); O != E; ++O) {
+      Instruction *op = dyn_cast<Instruction>(II->getOperand(O));
+      if (op && instructions.count(op)) {
----------------
Uppercase variable names.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:440
+
+  BranchInst* br = cast<BranchInst>(II);
+  Instruction *cmp = dyn_cast<Instruction>(br->getCondition());
----------------
Uppercase variable names.

Check if br is not null.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:460
+  for (II = BB2->begin(); !isa<TerminatorInst>(II); ++II) {
+    PHINode *PN = dyn_cast<PHINode>(II);
+    if (PN) {
----------------
You can combine the assignment and check into a single if:

```
if (PHINode *PN = dyn_cast<PHINode>(II))
```


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:481
+      Instruction *op = dyn_cast<Instruction>(I->getOperand(O));
+      if (op) {
+        if (op->getParent() == IfBlock1 || op->getParent() == IfBlock2)
----------------
Same here. Can combine the assignment and check into single if.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:505
+  // Accumulated latency of each instruction in their BBs.
+  std::map<Instruction *, int> BB1_instructions;
+  std::map<Instruction *, int> BB2_instructions;
----------------
How big are the keys of your map? Can you use llvm's map containers (like DenseMap, etc)?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:511
+
+  int new_BB_size = CountBBCodeSize(BB1, TTI) + CountBBCodeSize(BB2, TTI)
+                    + speculation_size;
----------------
Uppercase variable names.


https://reviews.llvm.org/D39352