Deinterleave: generalize to be lane mask-based

[LebedevRI]

Currently, it was implemented to only handle "deinterleaving"
(really, removal of not-demanded lanes) of lanes that can be
described by a linear polynominal. While this clearly worked,
there already were some FIXME comments, and it was already
clearly not generic-enough, e.g. it could not recurse into shuffles.

For proper handling of Reinterpret, which either merges adjacent lanes.
or splits lanes, this means that no support is possible at all.

[LebedevRI]

Note that this is not quite ready for full review.

[abadams]

This whole pass was written before we had nested vectors, and I don't believe it was ever updated to handle them, so it may not have behaved as expected when run during lowering (though I don't believe it was explicitly wrong)

When used in codegen, vectors have been flattened.

[LebedevRI]

On a second though..
@abadams thank you for taking a look, but this is a bit too much feedback, so i'm failing to parse it.
Let me focus on a single specific question:
given broadcast({1, 2, 3}, 3), which produces {1, 2, 3, 1, 2, 3, 1, 2, 3}, if we only demand i%2==0 lanes,
i.e. {1, 3, 2, 1, 3}, do we want to represent it as shuffle({broadcast({1, 2, 3}, 3)}, {0, 2, 4, 6, 8),
or as shuffle({1, 2, 3}, {0, 2, 1, 0, 2})?
Note that this is indeed nested vector specific. For non-nested vectors we don't have this issue.

[abadams]

Oh I see. I think we want the latter. I think there's never a reason to produce a shuffle of a broadcast. It should either be a single shuffle node, or a single broadcast node.

[LebedevRI]

Oh I see. I think we want the latter. I think there's never a reason to produce a shuffle of a broadcast. It should either be a single shuffle node, or a single broadcast node.

Thank you! Aha, that makes sense.

Now, the same question about the ramp.
Given a, b, c, d, which are scalars (lanes=1),
given ramp({a, b}, {c, d}, 3), which produces {a+0*c, b+0*d, a+1*c, b+1*d, a+2*c, b+2*d}
if we only want lanes 0, 1, 3 (!!! not a 1'st order polynominal, can not fold into ramp),
i.e. {a+0*c, b+0*d, b+1*d}, do we want to represent it as shuffle({ramp({a, b}, {c, d}, 3)}, {0, 1, 3),
or it's expanded form that does not involve computing the lanes that we will discard?

[abadams]

We want to be able to do things like load(shuffle(ramp)) -> shuffle(load(ramp)), so I think we should keep the ramps

[LebedevRI]

Okay, understood. Thank you.

[LebedevRI]

Ok, i think i finally got the ramp handling.
This could use some tests i suppose, but this should be basically it.

[LebedevRI]

@abadams all yours! :)
Will follow-up with Reinterpret handling after this is merged.

[LebedevRI]

This is a miscompile fix.
Since github is so hilarisouly atrocious at handling (or rather, not handling at all)
stacked/dependent PRs, i'm not sure if submitting this separately won't result in more mess.
Without this, one newly added test fails.

[abadams]

I'd a little bit confused about why this PR uses lists of demanded lanes but also masks. Why are both representations necessary?

[LebedevRI]

Neither is optimal for the problem at hand.
For example, it is O(1) to compute the number of demanded lanes given the list (vector) of demanded lanes,
but if you have a mask of demanded lanes, it's O(N).
Likewise, given a mask, it is trivial to upscale/downscale it (repeat/or-reduce bits),
but given indices it becomes somewhat less straight-forward.
Etc, etc.

We //could// settle for one, but code like this is exceptionally easy to be subtly broken,
which is why i've intentionally written the diff as it is now, very verbose.

[abadams]

I think upscaling indices is:

vector<int> upscale(const vector<int> &v, int scale) {
  vector<int> result(v.size() * scale);
  for (size_t i = 0; i < v.size(); i++) {
    for (int s = 0; s < scale; s++) {
      result[i*scale + s] = v[i] * scale + s;
    }
  }
  return result;
}

Which doesn't seem that much more complex than upscaling a mask. Can we switch to just indices?

[LebedevRI]

Okay, i've rewritten upscale_indices().

Though, that doesn't help much, because IndexXFormMapping::get() still does that roundtrip,
because the alternative is using a set/sort+unique a vector, and i can't possibly imagine that being better...

[abadams]

If that all it's for now, can it be made a private method in IndexXFormMapping called, e.g. sort_and_dedup_indices?

[LebedevRI]

I'm sorry, i do not follow. Against what are we defending?
It's already declared in .cpp. (tangentially, header story in halide seems really sad :/)
I already know i need it at least for downscale_indices_greedy(), though that can be avoided,
but i might also need it for the rest of reinterpret demanded mask forward processing.

[abadams]

I was proposing reducing the scope in which it's defined, and renaming it to make clear what its sole usage is, so that people reading the file understand more clearly what it's for. But if you're saying there are still other uses, then I guess don't do that.

Another minor comment on naming: I think "affine" is more common than "1st_order_polynomial" in compilers, and it's a little terser.

[LebedevRI]

(I ended up not needing it in another place, all hail #6908!)

My main concern is that said function has nothing specific to do with it's (only) user.
By that logic, we should also drop other single-use functions.
But to me that only hurts - they are there because they have obvious semantics,
and are separateable from users. Abstractions all the way, it's the only way to try to not sink.

[LebedevRI]

@abadams poke :) does my contrived reasoning sound convincing?

[abadams]

Yes, I'd much rather have compiler bugs cause internal assertion failures than silent problems for users. We get a more useful bug report that way.

What are you checking here? Is the concern that the check is expensive?

[LebedevRI]

My concern is that this check is expensive and statically dead, it's not going to ever fire.

[abadams]

For legibility, it might be worth an Affine struct with two ints in it instead of a std::pair.

[abadams]

Please add a comment. It took me a while to figure out what this function does.

[abadams]

I'm don't think these outputs are better than the old ones. The contain nested shuffles and look like they might codegen to unnecessary shuffle instructions.

[LebedevRI]

1. This is a subjective judgement. It would be good to know more specific, factual detail.
2. It really doesn't matter how we represent the shuffles, like or not, during codegen legalization, at least for X86, all shuffles are expanded and then new shuffles constructed from the basic element flow. https://godbolt.org/z/qW7x17aKs

[abadams]

On some platforms (Hexagon) we emit specific shuffle intrinsics that llvm preserves, and we're moving in the direction of handling more of instruction selection ourselves on all platforms (and finding ways to stop llvm from undoing our work).

To avoid subjective judgements, I'd really changes to be motivated by before/after assembly snippets that show this codegens better for some plausible pieces of input code. Otherwise I don't see what this complexity is buying us.

[LebedevRI]

we're moving in the direction of handling more of instruction selection ourselves on all platforms (and finding ways to stop llvm from undoing our work).

FWIW, this seems like extremely misplaced work. Has any thought been/being put into actually benefiting everyone and working on fixing the final source of truth? :)

[abadams]

Yes, we've thought about whether or not we should just be improving llvm's instruction selection, but there are a few problems with that approach. First, not all of our backends are llvm. E.g. the xtensa branch needs to do pretty involved fixed-point instruction selection and is not llvm-based. Hexagon is technically llvm, but in practice it's all opaque intrinsics that llvm just passes through.

Second, instruction selection is practically easier to do at the Halide level because it's a more restricted IR, for which we've developed a lot of relevant analysis tools (e.g. good bounds inference machinery). It's also practically easier because this is the code we understand and have control over.

The third more philosophical issue is that llvm's main performance goal is making naively written c++ code fast. When it comes to vector instruction selection, llvm is intentionally undoing our work, because it correctly believes that on average, it knows more about performance than some random C++ dev. It has been doing this by destructuring intrinsics in C++, deleting them entirely in LLVM IR, and replacing them with pattern matching. This can be brittle. Sometimes part of the pattern gets lifted as a loop invariant, or gets folded with a nearby shuffle, and instruction selection goes off the rails and you get bad code. Opening bugs for each case is a game of whack-a-mole.

Halide has very different users, so we're taking a different approach. Our approach has been to steadily add intrinsics inside the IR, first portable fixed-point ones (see FindIntrinsics.cpp), and now machine-specific vector instructions (See #6884). This means we can do instruction selection as IR->IR passes, and expose increasing amounts of control over instruction selection upwards to expert users. I don't think this approach would be welcome inside llvm.

[abadams]

I worry this PR is overkill. The main purpose of the deinterleave pass is to simplify things like f[ramp(0, 1, 16) / 2] to interleave(f[ramp(0, 1, 8)], f[ramp(0, 1, 8)]). If the demanded indices aren't affine, I don't think we can do anything much better than a single shuffle op, and doing more might just make it harder to get the compiler to generate the asm you want. Are there actual use-cases for the non-affine cases here where this implementation is better than the baseline of inserting a Shuffle node whenever you hit a non-affine case (e.g. a VectorReduce or Reinterpret node).

[LebedevRI]

I worry this PR is overkill. The main purpose of the deinterleave pass is to simplify things like f[ramp(0, 1, 16) / 2] to interleave(f[ramp(0, 1, 8)], f[ramp(0, 1, 8)]).

This is the common theme i'm encountering in this project. Zero documentation...
I think i'm rapidly plummeting back into "why do i even care" territory.

<...>

I think i'm going to go with the following reply:
are there any relevant benchmarks that could be used to objectively estimate the impact?

Affine or not is a red herring here, the actual problem, which seems to exist already,
is the fact that recursion can cause introduction of the shuffles to extract needed lanes,
once we stop being able to recurse further. Oh, and there's also the fact that
in doing so we either created irreparable mess of partially-full vectors,
or happen to split too-wide vectors into nice full vectors that result in optimal ILP.

[abadams]

We normally proceed by having some piece of code that's generating poor assembly, making a change to the compiler, and demonstrating that it's producing better assembly now and not regressing anything in the apps.

In the case here, I'm not sure if it's even possible to write front-end code that would trigger the new code paths, and that's why I'm wary of the PR. This is complex code that I will have to maintain five years from now, and I don't understand what the generality and complexity is buying us in terms of performance on some real use-case. If you don't have a use-case in mind, let's just do the simplest thing possible to make this pass correct for the new reinterpret semantics and move on to the next problem.

I'm sorry I'm being frustrating. We really appreciate your recent contributions.

[LebedevRI]

ban button is over there

I've contemplated not continuing this quickly derailed discussion, but.

We really appreciate your recent contributions.

The problem is that it appears i do not, since, well, clearly,
i'm still nudging this thread, which may have subj.

The problem is explicitly NOT that the diff in question is effectively dead.
All code is bad and broken, we just don't know about it yet.

My problem here is with, essentially, that i'm seemingly wasting time
seemingly without getting any closer to the virtual endgoal.
Yet things still go downhill from there.

My problem is with communication/miscommunication/quality of communication.

It was said that deinterleave needed to handle reinterpret,
so i've gone ahead, and due to the lack of any, even high-level, documentation,
interpreted that it's state was not the explicit design (give up early, give up often),
but an implementation artifact. So i've really gone full in, and under that assumption,
rewrote it in full generality without the affine restriction.

And yet, personally, i can only describe the long-awaited feedback
as total and complete lack of positive affirmation, lack of appreciation for the design,
taking it for granted, and effectively immediately discarding it.

So no, frustration does not quite describe the seething, irradiating, happiness that this has initially brought to me.

Again, and once again, i'm only describing my view on this. I don't know if it matches reality.