[PATCH] D110171: [VectorCombine] Switch to using a worklist.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:106
+    }
+    Worklist.pushValue(&Old);
+  }
----------------
lebedev.ri wrote:
> We also likely want to revisit users of the value of which we've just reduced the use-count.
> But honestly this is quite fragile, because generally speaking we want to revisit **all** it's uses transitively.
> https://bugs.llvm.org/show_bug.cgi?id=47238
Do you think it's worth doing this in the current patch? Not sure if it is possible to come up with a test case with the current limited set of combines.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1057
   bool MadeChange = false;
+  auto SimplifyInst = [this](Instruction &I) {
+    Builder.SetInsertPoint(&I);
----------------
spatel wrote:
> Nit: I'd name this "FoldInst" or "CombineInst" because I equate "Simplify" with "InstSimplify" and so not creating new values.
Changed to `FoldInst`.


================
Comment at: llvm/test/Transforms/VectorCombine/AArch64/load-extract-insert-store-scalarization.ll:21
   %lv = load <225 x double>, <225 x double>* %A, align 8
-  %ext.0 = extractelement <225 x double> %lv, i64 0
+  %ext.0 = extractelement <225 x double> %lv, i64 1
   %mul = fmul double 20.0, %ext.0
----------------
spatel wrote:
> Why change this test?
This was an accidental change that sneaked in. Removed now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110171/new/

https://reviews.llvm.org/D110171