This is an IR expansion pass intended to allow targets to opt-in to using the experimental reduction intrinsics introduced in D30086.
Its purpose is to see the effects of switching to the intrinsics in the IR, so this pass should be added to a target's pass config late, just before codegen. The expansion should result in the same shufflevector sequence form that targets currently expect reductions to be in.
Adding some target people - I think they ought to care about this more than I do. :-)
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #95820) | I'm not a huge fan of this - I would prefer not to rely on the invalidation semantics. Maybe collect all relevant instructions into a vector first, then do the replacement? |
| 84 ↗ | (On Diff #95820) | How do you expect this to happen? |
| 92 ↗ | (On Diff #95820) | What do we expect to happen in this case? |
| 96 ↗ | (On Diff #95820) | Please annotate the fallthrough here. (And perhaps it would be better to rewrite this to avoid it) |
| 105 ↗ | (On Diff #95820) | What about the internal instructions? |
| 117 ↗ | (On Diff #95820) | I'd expect a target query somewhere, regarding whether the intrinsic needs to be expanded. |
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #95820) | I can do that, but I've seen this kind of thing done before in other places. |
| 84 ↗ | (On Diff #95820) | Sorry, not entirely sure what you mean? This is an early exit if the given instruction isn't an intrinsic call. |
| 92 ↗ | (On Diff #95820) | As no in-tree target currently supports ordered reductions, and given that for SVE we want to enable support completely without using this expansion pass, I decided against trying to handle ordered reductions here. We just skip the intrinsic if we find it's an ordered reduction. If other targets want to experiment with ordered I think they can implement expansion via some scalarization method here. |
| 117 ↗ | (On Diff #95820) | My expectation was that targets wouldn't need, at least at first, that level of granularity. @RKSimon what do you think about this? |
Please add full context to the diff - especially as its dependent on another (in progress) patch.
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 117 ↗ | (On Diff #95820) | Are we guaranteeing that the reductions will match the ones supported by TargetTransformInfo::getReductionCost ? |
| test/CodeGen/Generic/expand-experimental-reductions.ll | ||
| 1 ↗ | (On Diff #95820) | I'd prefer to see the full reduction codegen here - regenerate with utils\update_test_checks.py ? |
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 117 ↗ | (On Diff #95820) | Do you mean useReductionIntrinsic()? If so, I suppose it comes down to the exact use case of this expansion. Michael originally asked for this so that targets could check the effect of using the intrinsics at the IR level only, and at a very late stage converting them into the shuffle form we have now. For that, I don't see why you would care about which individual intrinsics are expanded, rather than a simple on/off decision. If however there might be more uses of this, for example in future, if we want to enable intrinsic forms for *all* targets as a canonical form, and then use this pass with TTI to make a target dependent decision on which codegen-level form is preferred, then I think a TTI hook would make sense. I can add a hook anyway, perhaps defaulting to "expand all intrinsics" unless the target overrides it. |
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 96 ↗ | (On Diff #95820) | You've marked it as done by LLVM_FALLTHROUGH is still missing from these - you will get warnings on some buildbots |
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 96 ↗ | (On Diff #95820) | I might be misunderstanding what the "Done" means. I used it to mean I'll address this in the next patch when I upload it. I haven't got around to that yet. |
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #95820) | I'd suggest getting another reviewer's opinion on this. |
| 84 ↗ | (On Diff #95820) | Sorry, I misread this is dyn_cast<Instruction>, ignore. |
| 92 ↗ | (On Diff #95820) | The issue is that target-independent intrinsics are, by definition, supposed to be handled by any target. I shouldn't see a backend crash if I write IR that has the ordered intrinsic, and try to compile it for x86. Having said that - this is fine for now, but if we ever want to make these intrinsics non-experimental, this will have to be dealt with somehow. Please add a TODO. |
| 117 ↗ | (On Diff #95820) |
That's exactly what I'd expect, thanks. |
Addressed review comments, rewritten the pass a bit to be somewhat neater. D30086 is now committed now so this is ready to go if it looks ok.
LGTM, with a nit.
| lib/CodeGen/ExpandReductions.cpp | ||
|---|---|---|
| 137 ↗ | (On Diff #98285) | I don't believe you should ever be in the situation you don't have a TTI here. So it should be safe to just do: const auto *TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F); |