LGTM, please consider adding the following test:

Closing as we decided not to pursue this.

Shall we close this?

Thank you!

Is this good to go in?

In D58098#1403098, @dexonsmith wrote:

This seems like it could be useful. Do you have specific places in mind where you'd take advantage of it?

I've filed https://bugs.llvm.org/show_bug.cgi?id=40766 to track that.

Thanks for addressing the comments.

Hi Matt,

1. Addressed comments
2. Refined the switch's operand constraints with known bits and added corresponding tests (all based on real-world cases)

In D58096#1398829, @arsenm wrote:

It seems weird to me that doing this somewhere else somehow ends up being more expensive, but this needs a comment somewhere explaining why it should be handled here

In D58096#1394620, @arsenm wrote:

I'm not sure I see why LowerSwitch needs to worry about this optimization. Why doesn't SimplifyCFG or DCE or one of some other control flow optimization pass handle this so LowerSwitch doesn't have to worry about it?

I don't think this matters since the values should all be exactly representable.

In D57243#1385704, @arsenm wrote:

The type used in this case for the result has more to do with the type required for the operations after required for the legalization. The G_SUB can be done in either width, but the narrower sub seems like a more preferable canonical form.

In D57243#1385511, @qcolombet wrote:

Should we revert this change to have the conversation going or am I the only one unhappy with the absence of guarantees for the other operands?

In D56838#1362647, @hfinkel wrote:

In D56838#1362440, @rtereshin wrote:

In D56838#1361579, @hfinkel wrote:

Can we run EarlyCSE after CGP (instead of trying to do these kinds of point fixes)?

Hi Hal,

Thank you for looking into this!

That's a great suggestion! Originally I just eyeballed it as too expensive compile-time wise for the little effect that is achieved here.

This time around with you pointing it out, I performed the actual measurements for a very large suite of shaders (the downstream target in question is a GPU). I see about 1.0 (+/-0.3)% increase in overall compile time (the part of it that happens at an application run-time) on average. The generated code quality improvement that comes out of this may be important, but it doesn't trigger often enough to justify 1% compile time increase across the board.

Thanks,
Roman

Fair enough. Thanks for checking.

I have a comment about the test case, but otherwise LGTM.

Updated the test as requested

In D56838#1361579, @hfinkel wrote:

Can we run EarlyCSE after CGP (instead of trying to do these kinds of point fixes)?

Do you think it's better to remove the NFC tag from the patch? It doesn't look like it's completely NFC, though, I've tested this out for a major (though, out of tree) GPU target on a very large suite of shaders and found no difference.

LGTM, thanks for doing this!

I would tend to think that when we create instruction with constant we can discipline ourselves for not putting them on the LHS.

In D51362#1218451, @rtereshin wrote:

In D51362#1218394, @aemerson wrote:

I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?

Does that mean that something after the last InstCombine produces icmp's with the non-canonical operand order? If so maybe that something needs to be chased down and fixed instead.

In D51362#1218394, @aemerson wrote:

I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?

Hi Maxim,

Thank you!

Hi Aditya,

LGTM with minor nitpicks, please feel free to make changes you'll find reasonable while committing.

This is an impressive result, thanks for doing this!

LGTM

In D49660#1184980, @aivchenk wrote:

Hi Roman, I would prefer to do it as separate patches: the first one for Value splitting routine and the second for one splitToValueTypes

Hi Alexander,

Besides a nitpick, LGTM

In D49428#1175824, @volkan wrote:

LGTM

1. Renamed MaxRecurse to Depth as requested and updated the logic around it as MaxRecurse was going from the positive limit down to 0 which makes no sense for a parameter called Depth, so Depth is going from 0 to MaxDepth now.
2. Renamed RecursionLimit to MaxDepth to make the connection with Depth parameter clearer (which was MaxRecurse previously).

1. Added comments explaining partitioning of memory access instructions into groups of candidates for better performance
2. Added curly braces

In D48853#1173829, @efriedma wrote:

Yes, looks fine.

Hi Matt,

In D48853#1165748, @efriedma wrote:

I think if we're going to do this, we need to implement it on top of a SCEV-based known-bits implementation; introducing a separate getZeroExtendExprForValue API is going to lead to weird results if SCEV creates a zero-extend expression for some other reason.

Whether we should do this in general, I'm not really sure. I mean, yes, I can see how this particular form is a bit more convenient for the load-store vectorizer, but it doesn't seem very general; it seems more intuitive to canonicalize towards reducing the number of AddExprs. But maybe pulling as much information as possible outside of the zext is generally useful enough to make this worthwhile?

Do you also plan to implement a similar transform for AddRecs? (e.g. (zext i32 {1,+,2}<%while.body> to i64)).

Removed an out of place comment (the example comparing ConstantRange and KnownBits)

Updated and added comments as requested, renamed FullExpr to WholeAddExpr, and planned to add the following piece to the end of the commit message: