Page MenuHomePhabricator

[InstCombine] do not shrink switch conditions to illegal types (PR29009)
ClosedPublic

Authored by dendibakh on Nov 5 2018, 10:25 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dendibakh created this revision.Nov 5 2018, 10:25 AM
lebedev.ri added inline comments.
lib/Transforms/InstCombine/InstructionCombining.cpp
2485–2486 ↗(On Diff #172614)

The comments here and in the beginning of the test file say the same thing.
I remember there were a few mails about this on the mailing list.
Were you able to come to an conclusion that this is *certainly* not a backend problem?

dendibakh added inline comments.Nov 5 2018, 10:41 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
2485–2486 ↗(On Diff #172614)

Let me quote @spatel:

The backend isn't likely to produce good code for that anytime soon

That's why we decide to fix it on InstCombine, not the backend.
Anyway, I think I need to change the comment as well.

lebedev.ri added inline comments.Nov 5 2018, 10:44 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
2485–2486 ↗(On Diff #172614)

Okay, makes sense i guess.
Please also make sure to file a bug for backends, and for this regression,
and highly ideally make sure that there are backend tests showing their deficiency.

dendibakh updated this revision to Diff 172617.Nov 5 2018, 10:58 AM

Changed the comment.

Please do reference the backend bug№ in the comments.

test/Transforms/InstCombine/narrow-switch.ll
6–7 ↗(On Diff #172617)

Here too.

To provide more background/motivation for why I don't think we will solve the underlying problem soon, we can fork this example into its own bug:
https://bugs.llvm.org/show_bug.cgi?id=29009#c4

I said that was an instcombine opportunity in that bug comment, but that was a mistake. It probably requires a GVN-level-of-complexity to see that kind of equivalence between the 2 different-sized masked values. But AFAICT, neither -gvn nor -newgvn optimizes that today (so we should file a 2nd bug to track that as an IR opportunity).

I'm not sure where/how we would catch that in the backend, but that's the kind of code we would see after legalization of non-power-of-2 types.

So that's the reason I think we're justified in limiting this transform with shouldChangeType(). In an ideal world, we could let instcombine go wild with illegal types, but as this example shows, getting optimal codegen from that is likely difficult (and it might not be handled well by other IR passes too), so that's why instcombine has this limitation.

dmgreen added a subscriber: dmgreen.Nov 6 2018, 1:40 AM
dendibakh updated this revision to Diff 172792.Nov 6 2018, 10:22 AM

Fixed comments.

Looks good.

lib/Transforms/InstCombine/InstructionCombining.cpp
2485–2486 ↗(On Diff #172614)

Minor: when you fix the inline comment, please do mark it as fixed (the checkbox -------------------------------------------^)

dendibakh marked 2 inline comments as done.Nov 6 2018, 11:21 AM

@spatel, @lebedev.ri, if no other objections, can you please commit it on my behalf?

spatel accepted this revision.Nov 7 2018, 5:45 AM

LGTM - I can commit this for you.

This revision is now accepted and ready to land.Nov 7 2018, 5:45 AM
This revision was automatically updated to reflect the committed changes.