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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:34
+  // Only replace operands that can be passed-by-value.
+  if (!Ty->isFirstClassType())
+    return false;
----------------
aeubanks wrote:
> does `isa<Instruction> || isa<GlobalValue>` work, instead of these various checks?
Just `isa<GlobalValue>` unfortunately does not catch constants such as `bitcast (i32* @Global to float*)`.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:46
+  // Do not replace literals.
+  if (isa<ConstantData>(Val))
+    return false;
----------------
aeubanks wrote:
> if this pass runs after the `operands` pass and the `operands` pass reduces a bunch of things to `undef`, doesn't that mostly defeat the purpose of this since we're skipping constants?
> and the description mentions that the instruction reduction pass should hopefully clean up dead instructions, but that runs before this pass
> 
> 
I myself do not run anything that results in undefs being introduced using the `--passes` switch. Which passes run are enabled by default currently cannot be defined independently of which passes are available. I myself run llvm-reduce in a fixpoint loop until no more reductions are found, which is already necessary now to get a maximally reduced output. 

I'd work on upstreaming more patches supporting this workflow. Until then maybe you could suggest an alternative position in the current pass pipeline? In any case, there is still use in running it after the `operands` pass in reducing arguments that cannot just set to Undef (or zero with in `D110274`), but needs a non-constant value.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111503