This is an archive of the discontinued LLVM Phabricator instance.

[llvm-reduce] Reducing call operand bundles
ClosedPublic

Authored by lebedev.ri on Jul 5 2020, 8:33 AM.

Details

Summary

This would have been marginally useful to me during/for rG7ea46aee3670981827c04df89b2c3a1cbdc7561b.

With ongoing migration to representing assumes via operand bundles on the assume, this will be gradually more useful.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jul 5 2020, 8:33 AM

I must say, i found the reduction infra to be a little bit rough.
What do people think about the following:

llvm/lib/IR/Instructions.cpp
251

unused param?

llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
35

if you move this up, then you don't need to wrap the forward declaration of class Module above.

79

explicit for one operand constructors?

lebedev.ri marked 3 inline comments as done.

@nickdesaulniers thank you for taking a look!
Addressing review nits.

llvm/include/llvm/IR/InstrTypes.h
1145

If we're going to create a new interface to CallBase, I kind of want to use it in more than just one place. In particular, at least InlineFunction() in llvm/lib/Transforms/Utils/InlineFunction.cpp looks like a perfect candidate to use this. There may be more, if you grep for the casts. In that way, this change might help DRY up and also remove a repetitious pattern. WDYT?

lebedev.ri marked 2 inline comments as done.

Split CallBase::Create() into a separate patch.

lebedev.ri added inline comments.
llvm/include/llvm/IR/InstrTypes.h
1145

Doesn't really sound interesting to me.

nickdesaulniers accepted this revision.Jul 6 2020, 12:12 PM

Doesn't really sound interesting to me.

LOL, then why did you split it?

This revision is now accepted and ready to land.Jul 6 2020, 12:12 PM
lebedev.ri marked an inline comment as done.

Actually upload up-to-date patch now.

llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
35

Hold up, that's actually bogus.
https://godbolt.org/z/0qTwwz

llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
35

or just include the appropriate header for llvm::Module and forget forward declaration?

lebedev.ri marked 2 inline comments as done.Jul 6 2020, 12:35 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
35

But here we don't need to know what Module actually is though, just a pointer to it.
The current include list is what IWYU suggests,
and i believe cleanup elsewhere in codebase is ongoing in the same direction.

Tyker added a comment.Jul 6 2020, 1:01 PM

Thank you for reducing rG7ea46aee3670981827c04df89b2c3a1cbdc7561b
this seems quite useful.

llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

Maybe we should make CallsToRefine a MapVector since the association from a index to a Bundle depends on its order in the map.
and the key depends on a pointer value that will change when the Module gets cloned.

lebedev.ri marked 3 inline comments as done.Jul 6 2020, 1:07 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

No, the current logic is correct, that map doesn't live that long.

Tyker added inline comments.Jul 6 2020, 1:41 PM
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

here is the situation i think fails.

extractOperandBundesFromModule is called a first time and generate a reduction.
the reduction isn't considered interesting.
extractOperandBundesFromModule is called a second time with different chunks to keep.
but the association from an index to a Bundle is different from the first time because the module isn't the same
the association from a index to a Bundle isn't the same,
so extractOperandBundesFromModule can remove the same operand bundle a second time and not have tried every operand bundles at the end of the passe.

lebedev.ri marked 2 inline comments as done.Jul 6 2020, 1:56 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

In reduceOperandBundesDeltaPass(), we tell runDeltaPass() how many features (here: bundles)
we have in this module M0. It then comes with different chunks to keep and tells us
to mutate the module Mc (which is a perfect clone of M0).

It is up to us to actually enumerate the features (here: bundles).
As long as the mapping is stable, i.e. we get the same result
when calling extractOperandBundesFromModule() on the same input,
we're good.

Tyker added inline comments.Jul 6 2020, 2:09 PM
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

when calling extractOperandBundesFromModule() on the same input, we're good.

i agree but runDeltaPass makes clones of modules before giving them to extractOperandBundesFromModule to modify.
so the pointer values of Modules will not be the same across runs of extractOperandBundesFromModule even if the Modules are identical.

lebedev.ri marked 2 inline comments as done.Jul 6 2020, 2:19 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

Sure, but can you point me at the spot where you believe that would matter?

Tyker added inline comments.Jul 6 2020, 2:21 PM
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

the key of the Map depends on pointer values

lebedev.ri marked 3 inline comments as done.Jul 6 2020, 2:37 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

R.CallsToRefine is indeed a map, with key being CallBase *.
We *just* built that map, for this very Mc we were provided.
I'm still not seeing where pointer order would matter..

Tyker added inline comments.Jul 6 2020, 2:50 PM
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

the map depends on pointer values so its ordering is different on each run.
the map is iterated upon and the iteration order affects the mapping from Index to Feature(Bundles)
so every run of has a different mapping.

lebedev.ri marked 3 inline comments as done.Jul 6 2020, 2:54 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

Ahh, that is what you mean. No, it's the other way around.
Do you agree that the only place we iterate over the map is:

for_each(R.CallsToRefine, [](const auto &P) {
  return maybeRewriteCallWithDifferentBundles(P.first, P.second);
});

?

But as you can see in maybeRewriteCallWithDifferentBundles(),
it's second param is the indexes of bundle to preserve.
So we have already performed the mapping before iterating over the map.

Tyker added inline comments.Jul 6 2020, 3:09 PM
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

you are right, sorry for my confusion.

lebedev.ri marked 4 inline comments as done.Jul 6 2020, 3:14 PM
lebedev.ri added inline comments.
llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp
139

NP, thank you for taking a look!

This revision was automatically updated to reflect the committed changes.
lebedev.ri marked an inline comment as done.