[PATCH] D33380: Add a dominanance check interface that uses caching for instructions within same basic block.

Davide Italiano via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat May 20 13:50:30 PDT 2017


davide added a comment.

This is a really nice cleanup and I think we can greatly benefit from it in other passes as well :)
Some comments inline.



================
Comment at: lib/Transforms/Utils/OrderedInstructions.cpp:10
+//
+// This file defines utility to check dominance information for 2 instructions.
+//
----------------
I'd say `check dominance relation between two instructions` rather than `information` (and I'd add efficiently, as we take shortcuts here :))


================
Comment at: lib/Transforms/Utils/OrderedInstructions.cpp:17-18
+
+#define DEBUG_TYPE "ordered-insts"
+
+bool OrderedInstructions::dominates(const Instruction *InstA,
----------------
Do you need to define `DEBUG_TYPE` ?
This doesn't use any `DEBUG()` statements


================
Comment at: lib/Transforms/Utils/OrderedInstructions.cpp:19
+
+bool OrderedInstructions::dominates(const Instruction *InstA,
+                                    const Instruction *InstB,
----------------
I'd add a comment to this explaining what it does/how.


================
Comment at: lib/Transforms/Utils/OrderedInstructions.cpp:30-31
+    return OBB->second->dominates(InstA, InstB);
+  } else
+    return DT->dominates(InstA->getParent(), InstB->getParent());
+}
----------------
braces around this `else`, for consistency.


================
Comment at: unittests/Transforms/Utils/OrderedInstructions.cpp:16
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/LegacyPassManager.h"
+#include "llvm/IR/Module.h"
----------------
Do you need to include `LegacyPassManager.h` ?


================
Comment at: unittests/Transforms/Utils/OrderedInstructions.cpp:30-31
+
+  // Create BBX with 2 loads.
+  BasicBlock *BBX = BasicBlock::Create(Ctx, "bbx", F);
+  B.SetInsertPoint(BBX);
----------------
Do you mind to add a textual representation of these tests for the reader? (it doesn't need to be perfect, just as guidance).


https://reviews.llvm.org/D33380





More information about the llvm-commits mailing list