[PATCH] D38313: [InstCombine] Introducing Aggressive Instruction Combine pass

[Amjad Aboud via Phabricator via llvm-commits]

aaboud marked 15 inline comments as done.
aaboud added a comment.

Thanks Zvi for the comments.
I will upload a new patch with most of the comments fixed.
See few answers below.



================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:54
+void AggressiveInstCombiner::getAnalysisUsage(AnalysisUsage &AU) const {
+  AU.setPreservesCFG();
+  AU.addRequired<TargetLibraryInfoWrapperPass>();
----------------
zvi wrote:
> Is there an importance of order to the calls? If not, maybe group the set/addPreserve* calls and addRequired call to two group separated by a newline. IMHO it's more readable.
Not sure if there is any importance to the order.
I just did what InstructionCombiningPass does!


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:40
+/// \p New instruction DebugLoc and Name with those of \p Old instruction.
+static void InsertAndUpdateNewInstWith(Instruction &New, Instruction &Old) {
+  assert(!New.getParent() &&
----------------
zvi wrote:
> +1 for preserving DebugInfo. Is there other metadata that may need to be copied? I can't think of anything in particular, just wanted to raise the possibility.
This is what InstCombine preserves.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:44
+  BasicBlock *BB = Old.getParent();
+  BB->getInstList().insert(Old.getIterator(), &New); // Insert inst
+  New.setDebugLoc(Old.getDebugLoc());
----------------
zvi wrote:
> Is it possible to reuse llvm::ReplaceInstWithInst instead of doing the low-level work explicitly?
llvm::ReplaceInstWithInst also deletes the old instructions, which we are not ready to delete at this point.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:64
+  case Instruction::Xor:
+    Ops.push_back(I->getOperand(0));
+    Ops.push_back(I->getOperand(1));
----------------
zvi wrote:
> Could we avoid pushing constants and maybe even generalize to avoid values that are not instructions? At least for these cases it may be ok, not for others, such as divide where we need to be more cautious
I prefer not to complicate this function, it should return operands that are related to the optimization, the caller should check each relevant operand if it is a constant or instruction, there will be cases where even a constant need to be evaluated and will not skipped immediately.


https://reviews.llvm.org/D38313