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

[Tyker via Phabricator via llvm-commits]

Tyker 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);
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83177