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

[Zvi Rackover via Phabricator via llvm-commits]

zvi added a comment.

Amjad, some questions about where do we want this work to evolve to.

Looking at the following example:

  define i16 @foo(i8 %B, i16 %A, i1* %pbit) {
             %zext = zext i8 %B to i32
           %mul = mul i32 %zext, %zext
         %LSB = and i32 %mul, 255 ; <----------- First use
       %cmp = icmp ne i32 %LSB, 0
     store i1 %cmp, i1* %pbit
         %sext = sext i16 %A to i32
       %and = and i32 %mul, %sext ; <---------- Second use
     %trunc = trunc i32 %and to i16
     ret i16 %trunc
   }

The indentation should help with seeing the two branches in the expression DAG. The 'mul' is used by both 'and' instructions.
The most 'type-shrunken' form with no duplications should be this:

define i16 @foo(i8 %B, i16 %A, i1* %pbit) {

  %zext = zext i8 %B to i16
  %mul = mul i16 %zext, %zext
  %LSB = and i16 %mul, 255
  %cmp = icmp ne i16 %LSB, 0
  store i1 %cmp, i1* %pbit, align 1
  %and = and i16 %mul, %A
  ret i16 %and

}

I assume that this patch will not cover this case because the bottom-up traversal starting from the 'trunc' will only visit one branch of the two.  The bail-out condition " if (UI != CurrentTruncInst && !InstInfoMap.count(UI))

  return nullptr;" in getBestTruncatedType() will fire because one of the 'mul' users will not be mapped in the expressions DAG. If, for example, the expression DAG were constructed by traversing both defs and uses, it would have covered the entire function in the above example, and there would be sufficient information to deduce that both branches of the expression DAG can be shrunk.

Does this make sense?



================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:62
+  const DataLayout &DL;
+  AssumptionCache *AC;
+  const DominatorTree *DT;
----------------
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


================
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();
----------------
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.


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:260
+  } else { // MinBitWidth == ValidBitWidth
+    bool FromLegal = MinBitWidth == 1 || DL.isLegalInteger(OrigBitWidth);
+    bool ToLegal = MinBitWidth == 1 || DL.isLegalInteger(MinBitWidth);
----------------
Please add a comment explaining the considerations for bailing out when MinBitWidth == ValidBitWidth


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:270
+  // can't shrink something that has multiple users, unless all users are
+  // dominated by the trunc instruction, i.e., were visited during the
+  // expresion evaluation.
----------------
I am commenting about this because it got me a bit confused. I think the correct term would be post-dominated or "users that dominate the truncate instruction". 


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:271
+  // dominated by the trunc instruction, i.e., were visited during the
+  // expresion evaluation.
+  for (auto Itr : InstInfoMap) {
----------------
*expression


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:311
+  Instruction *I = cast<Instruction>(V);
+  assert(InstInfoMap[I].NewValue);
+  return InstInfoMap[I].NewValue;
----------------
Better call .lookup() rather than operator[] to avoid side effects in asserts


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:320
+    // Check if this instruction have already been evaluated.
+    assert(!InstInfoMap[I].NewValue);
+
----------------
Better call .lookup() rather than operator[] to avoid side effects in asserts
312	


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:358
+    // Update Worklist entries with new value if needed.
+    if (auto *NewCI = dyn_cast<TruncInst>(Res)) {
+      auto Entry = std::find(Worklist.begin(), Worklist.end(), I);
----------------
Consider moving this 'if' block  under the appropriate case statement above.


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:359
+    if (auto *NewCI = dyn_cast<TruncInst>(Res)) {
+      auto Entry = std::find(Worklist.begin(), Worklist.end(), I);
+      if (Entry != Worklist.end())
----------------
STLExtras.h can help with saving a few bytes of code :)

```
auto Entry = find(Worklist, I);
```


https://reviews.llvm.org/D38313