[PATCH] D111818: [llvm-reduce] Introduce operands-skip pass.

Arthur Eubanks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 14 17:31:14 PDT 2021


aeubanks added a comment.

this looks very cool!



================
Comment at: llvm/test/tools/llvm-reduce/operands-skip.ll:2
+; RUN: llvm-reduce %s -o %t --delta-passes=operands-skip --test FileCheck --test-arg %s --test-arg --match-full-lines --test-arg --check-prefix=INTERESTING --test-arg --input-file
+; RUN: FileCheck %s --input-file %t --check-prefixes=REDUCED --dump-input-context=999 -vvv
+
----------------
leftover debugging?


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:22
+/// Collect all values that are directly or indirectly referenced by @p Root,
+/// including Root itself. This is a BF search such the more the more steps need
+/// to get to the reference, the more behind it is found in @p Collection. Each
----------------



================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:48
+static int classifyReductivePower(Value *V) {
+  if (isa<UndefValue>(V))
+    return 7;
----------------
I thought we preferred ConstantData over undef


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:75
+/// Calls @p Callback for every reduction opportunity in @p F. Used by
+/// countOperands() and extractOperandsFromModule() to ensure consistenct
+/// between the two.
----------------
consistency


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:77
+/// between the two.
+template <class T> static void opportunities(Function &F, T Callback) {
+
----------------
I'd prefer a `function_ref<void(Use &, ArrayRef<Value *>)>` over templates


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:80-83
+  FunctionAnalysisManager FAM;
+  FAM.registerPass([]() { return DominatorTreeAnalysis(); });
+  DominatorTreeAnalysis DTA;
+  DominatorTree &&DT = DTA.run(F, FAM);
----------------
`DominatorTree DT(F);`
the pass manager infra doesn't give us anything here


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:85
+
+  // Return whether @p LHS is "more reduced" that @p RHS. That is, wheter @p RHS
+  // should be preferred over @p LHS in a reduced output. This is a partial
----------------



================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:116
+    // Compress the number of used arguments by prefering the first ones. Unused
+    // traling argument can be removed by the arguments pass.
+    auto *LHSArg = dyn_cast<Argument>(LHS);
----------------



================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsSkip.cpp:132
+      // Collect refenced values as potential replacement candidates.
+      SetVector<Value *> &&ReferencedVals = collectReferencedValues(OpVal);
+
----------------
is `&&` necessary? the result of `collectReferencedValues()` should be moved into a local variable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111818



More information about the llvm-commits mailing list