[PATCH] D111503: [llvm-reduce] Introduce operands-to-args pass.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

generally sounds good to me - though I'll probably leave more detailed review/approval to other folks who have been working more closely on llvm-reduce



================
Comment at: llvm/test/tools/llvm-reduce/operands-to-args.ll:17-21
+; Do not add any arguments for %Keep and @GlobalKeep.
+; EXCITING: %[[KEEP:LocalKeep[0-9]*]] = add i32 %k, 21
+; EXCITING: store i32 %[[KEEP]], i32* @GlobalKeep, align 4
+
+; EXCITING-LABEL: define void @func_caller() {
----------------
I think the dominant terminology here is "interesting" rather than "exciting" - probably best to keep with that for consistency?


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:21-29
+  for (Use &Op : F->uses()) {
+    // Can only replace direct calls to the function.
+    if (auto *Call = dyn_cast<CallBase>(Op.getUser()))
+      if (Call->getCalledFunction() == F)
+        continue;
+
+    return false;
----------------
could potentially rephrase this with `llvm::all_of`


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:57-58
+
+  SmallVector<Value *> Args;
+  SmallVector<OperandBundleDef> OperandBundles;
+  for (CallBase *CI : Callers) {
----------------
Generally I'd sink these into the loop (usual keeping variables with the smallest scope necessary - simplifies the code/logic/reasoning) unless there's especially strong evidence that the memory reuse has observable performance improvements?

Alternatively, could the new arguments be created once, and the call-site-specific argument be copied over the start of the SmallVector, without the need to clear/rebuild it each time?


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:170-178
+  SmallVector<Function *> Funcs;
+  for (auto &F : Program->functions()) {
+    if (!canReplaceFunction(&F))
+      continue;
+    Funcs.push_back(&F);
+  }
+
----------------
Any particular reason these loops can't be combined (removing the "Funcs" SmallVector in the process) like the `countOperands` function below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111503