[PATCH] D39267: [GISel]: Change Legalization from top down to bottom up + DCE

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

Hi Aditya,

As we discussed privately, looking forward with our plans to separate out the combiner mechanics, we should be extra careful here about our legalisation/combiner iteration scheme. From a design point of view, I'd like to see stricter separation between legalisation and combiner phases.[1] What do you think about changing the overall algorithm to instead run (legalize(), combine()) until we reach a fixed point? It would also be nice if we can share the same iteration scheme between legalizer's combiner and the separate combiner pass. That is, since you already compute the post-ordering of the blocks, we can just reverse them to have our ordering for the combine phase, and that would match the intended ordering for the combiner pass.

[1] To fill in everyone about our discussions: we plan to split the combiner mechanics into a separate pass/component, with the intention that the core routines can be shared between multiple clients. However, as a result of the GISel legaliser, we still have "artefacts" generated which, without optimisation before re-legalisation, could cause poor codegen. Therefore we need the ability for the legaliser to invoke some subset of the future GI-combiner/optimizer to clean these up.

Side note: it'll be easier to review codegen changes if we use the update test script in a separate commit first.

Cheers,
Amara



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerCombiner.h:202
       return tryCombineMerges(MI, DeadInsts);
+    case TargetOpcode::G_TRUNC: {
+      bool Changed = false;
----------------
I'm not sure about this approach to integrating combiners with the legalizer. Since we process the blocks and instructions bottom up with this change, having the combiner rather un-intuitively look at instructions top-down, and only in some cases, makes this harder to reason about.

Another thing is if we're going to separate out the combiner routines then we shouldn't have inconsistency between matching behaviour between different combiners.


https://reviews.llvm.org/D39267