[PATCH] D38313: [InstCombine] Introducing Pattern Instruction Combine plug-in into InstCombine pass

[Amjad Aboud via Phabricator via llvm-commits]

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

Thanks Zvi for the comments.
I fixed most of them and will upload a new patch soon.



================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:62
+  const DataLayout &DL;
+  AssumptionCache *AC;
+  const DominatorTree *DT;
----------------
zvi wrote:
> Didn't see any actual uses of AC and DT. Will they be used in a follow-up patch? Even if yes, better remove to avoid breaking -Werror builds
Indeed, I will be using them in next patches.
I do not think there is a warning/error issue with unused class members, but I will remove them from this patch and add them later when they are actually used.



================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:139
+/// that instruction, with respect to the Trunc expression dag optimizaton.
+static void getRelevantOperands(Instruction *I, SmallVectorImpl<Value *> &Ops) {
+  unsigned Opc = I->getOpcode();
----------------
zvi wrote:
> Instead of populating a container, considering returning a pair of begin,end iterators to the operands of interest (or a op_range). Of course, this will only work if the operands of interest are consecutive.
It will not work for the PHINode instruction.


https://reviews.llvm.org/D38313