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

[Zia Ansari via Phabricator via llvm-commits]

zansari added a comment.

Hi Amjad,

I didn't do a thorough drill down of all the changes (I'll leave that to Craig and Sanjay), but skimmed through it. The design seems reasonable, as long as others are ok with the instcombine plugin.
I can't tell all the effects this will have, but take care that the zext->(operations...)->trunc optimization doesn't remove a zext on a load that feed into an operation such that we turn a "movz[bw] mem -> reg" into "mov[bw] mem -> [bw]reg", since this can introduce extra merge/false dependence hits on most newer Intel architectures.

Zia.



================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:146
+  case Instruction::SExt:
+    // trunc(trunc(x)) -> trunc(x)
+    // trunc(ext(x)) -> ext(x) if the source type is smaller than the new dest
----------------
Are you planning on doing these peepholes here? If not, is there any other reason for leaving these cases in this switch?


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:180
+  switch (Opc) {
+  case Instruction::Trunc:
+  case Instruction::ZExt:
----------------
Same as above. Any reason for not breaking for trunc/zext/sext?


https://reviews.llvm.org/D38313