This is an archive of the discontinued LLVM Phabricator instance.

[llvm-reduce] Introduce operands-to-args pass.
ClosedPublic

Authored by Meinersbur on Oct 9 2021, 8:47 PM.

Details

Summary

Instead of setting operands to undef as the operands pass does, convert the operands to a function argument. This avoids having to introduce undef values into the IR which have some unpredictability during optimizations.

For instance,

define void @func() {
entry:
  %val = add i32 32, 21
  store i32 %val, i32* null
  ret void
}

is reduced to

define void @func(i32 %val) {
entry:
  %val1 = add i32 32, 21
  store i32 %val, i32* null
  ret void
}

(note that the instruction %val is renamed to %val1 when printing the IR to avoid ambiguity; ideally %val1 would be removed by dce or the instruction reduction pass)

Any call to @func is replaced with a call to the function with the new signature and filled with undef. This is not ideal for IPA passes, but those out-of-scope for now.

Diff Detail

Event Timeline

Meinersbur created this revision.Oct 9 2021, 8:47 PM
Meinersbur requested review of this revision.Oct 9 2021, 8:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2021, 8:47 PM

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

llvm/test/tools/llvm-reduce/operands-to-args.ll
18–22

I think the dominant terminology here is "interesting" rather than "exciting" - probably best to keep with that for consistency?

llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp
22–30

could potentially rephrase this with llvm::all_of

58–59

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?

171–179

Any particular reason these loops can't be combined (removing the "Funcs" SmallVector in the process) like the countOperands function below?

Meinersbur marked an inline comment as done.
  • EXCITING->INTERESTING
  • sink SmallVector declaration into loop.
  • Use all_of
Meinersbur marked 2 inline comments as done.Oct 10 2021, 2:28 PM
Meinersbur added inline comments.
llvm/test/tools/llvm-reduce/operands-to-args.ll
18–22

I used exciting/boring already for D110534. The reason is that interesting is a substring of uninteresting and therefore pretty much unusable with FileCheck. It does not apply here, so I changed it to INTERESTING.

llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp
171–179

The Module's function list is modified in substituteOperandWithArgument and invalidates iterators.

  • Use make_early_inc_range
  • Consistent (non-)use of auto
llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp
171–179

Using make_early_inc_range now to avoid the invalidated iterator.

  • Fix function passed to calll of itself.
  • Fix SmallVectorBase::set_size();
  • Be more permissive with what can be replaced. In particular, allow replacing constants derived from GlobalValues.

Generally sounds about right to me - I'll leave final sign off to other folks more connected to llvm-reduce.

aeubanks added inline comments.Oct 11 2021, 3:58 PM
llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp
22

unused F

32

leftover debugging?

35

does isa<Instruction> || isa<GlobalValue> work, instead of these various checks?

47

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

Meinersbur marked an inline comment as done.
  • Remove leftovers
llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp
35

Just isa<GlobalValue> unfortunately does not catch constants such as bitcast (i32* @Global to float*).

47

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.

aeubanks accepted this revision.Oct 12 2021, 1:50 PM
aeubanks added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp
47

Makes sense.

This revision is now accepted and ready to land.Oct 12 2021, 1:50 PM
This revision was automatically updated to reflect the committed changes.