Hi Sanjay,

Doesn't look like this just handles --llc-binary command line argument of update_llc_test_checks.py correctly: I assume references to llc from within the preprocess_cmd should also be replaced with the value of --llc-binary if any, similarly to the substitution done for the main part of the command?

My suggestion is to keep making progress here:

1. move the check out of is profitable to processBlock top level
2. put a comprehensive comment on it outlining the issues discussed here (and off fabricator) so far
3. do (2) in the test as well (and keep the test otherwise as is)

Addressing the feedback (tidying up the comments a bit as discussed, going for if(! continue; instead of if() {})

Trimming down the comments a little bit as requested.

Addressing the feedback, NFC.

Adding // Adding Use to ArtifactList. comment in front of WrapperObserver.changedInstr(Use); as requested, NFC.

missed a test case update (arm64-fallback.ll)

Added a TableGen test as requested, though, I'm quite convinced it wasn't worth the time and will be difficult to maintain.

In D69653#1728175, @arsenm wrote:

Testcase?

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!