[PATCH] D83177: [llvm-reduce] Reducing call operand bundles

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceOperandBundles.cpp:138
+
+  for_each(R.CallsToRefine, [](const auto &P) {
+    return maybeRewriteCallWithDifferentBundles(P.first, P.second);
----------------
Tyker wrote:
> lebedev.ri wrote:
> > Tyker wrote:
> > > lebedev.ri wrote:
> > > > Tyker wrote:
> > > > > lebedev.ri wrote:
> > > > > > Tyker wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > Tyker wrote:
> > > > > > > > > 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.
> > > > > > > > No, the current logic is correct, that map doesn't live that long.
> > > > > > > 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.
> > > > > > > 
> > > > > > > 
> > > > > > 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.
> > > > > > 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.
> > > > Sure, but can you point me at the spot where you believe that would matter?
> > > the key of the Map depends on pointer values
> > `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..
> 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.
> 
> 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83177