Updated after D69975 was reviewed and committed.
Now showing the codegen improvements related with this patch

This is now commited to the github repo
https://github.com/llvm/llvm-project/commit/fe0763d28a572f72007637c7bd097bc19cbb58fc

Hi @spatel, @asl
Thanks for reviewing this. I'm getting some trouble at committing to the upstream repo in the way I would like. I asked for help here first, but then I thought it was better to use llvm-dev instead to avoid clutter here. So this is an edit removing my previous comment. Thanks again

Following the above conversation, I created D69975 as the base tests before this patch can be committed.
I put the base tests in a separate patch to actually show the differential effects of this one.
I will open this for review after D69975 is reviewed.
Thanks,

In D69326#1736956, @spatel wrote:

Usually, a first commit will fix a code comment with a spelling error or formatting problem and be "NFC" (no-functional-change). Feel free to try that to make sure things are working.

In D69326#1737100, @spatel wrote:

You will want to rebase this patch either way - I had to do some cleaning just to step through this function:
rG2fdd58c5066f
rG777d1d1d9811

Ok, no problem.

@spatel Thanks for reviewing and commiting my previous patches on this subject. I have now been given commit access. I have not tried yet, but it should be good.

Made sure diff matches latest trunk changes

@asl Ok thanks, I agree that 9 bit shifts may also benefit from the version with amount.
If this is ok, I will propose the TLI function replacement on a different patch after this is reviewed, to keep things organised.

Updated diff to account for the latest trunk changes

In D69609#1726965, @spatel wrote:

LGTM - have you requested/acquired commit privilege yet?

@asl I identified you as the creator of the MSP430 patch that optimised 8 bit and larger shifts by combining them with bswap and extend instructions. This creates relatively cheap instruction sequences for exactly 8 bit shifts.

@spatel I updated this revision diff according to your suggestions. I also created a new baseline test revision to help identify the effects of this patch. I hope this is ok. Thanks in advance for your time.

I created a new baseline revision, D69609, that help to better reflect the effects of applying this patch. The referred revision should therefore be applied before this one for consistency reasons.

Updated tests to reflect each covered case in an independent way with more focused IR patterns.
Added comments to the test file that refer to the transformation we are dealing with.

Code style correction

Made sure the updated diff file accounts for recent trunk changes

Move repeated calls to N0.getValueType() into a variable

In D69116#1715005, @spatel wrote:

LGTM - see inline for a minor improvement.

Do you have commit access?

Updated with latest suggestions

Follow ups in D69099 , D69116 and D69120

Should be ok now.

@spatel
I just realised that I somehow messed this test file with the already optimised one. Please discard this until I upload the right one. Thanks

Updated baseline test file with the given recommendations:

• removed explicit target and datalayout from .ll test file
• removed dso_local

I know this is possibly not the right place to ask this particular question, but I thought that I would get a more focused reply by asking here:

In D68982#1710606, @asl wrote:

@joanlluch There are two things here. We understand that there might be some out-of-tree targets that would benefit from changes in the mainline. However, it is still important to add tests that utilize mainline targets as this ensures that the proposed changes won't e.g. break during some refactoring, etc. So, using MSP430 (probably the smallest) / AVR is perfectly fine for tests, etc.!

In D68982#1710529, @asl wrote:

The MSP430 changes / overall direction looks extremely useful to me. We just need to ensure everything is done properly. This is exactly what @spatel proposed.

I will be happy to review all MSP430 stuff :)

Hi,
Sorry, I somehow missed your post before I posted my last one. Thanks for that.

Hi @spatel
Thank you for your support. I'll try to do what you suggest regarding generating of baseline tests and splitting the patch.

I abandoned this, created D68982 instead

@spatel I want to express my support to selects over bit manipulation instructions in IR, as stated above, in order to move such optimisations to DAGCombine. Ideally, this should involve the removal of some of the existing InstCombineSelect transformations, particularly most of the ones in foldSelectInstWithICmp. However, as I exposed earlier in LLVM-dev, the DAGCombine code should incorporate hooks to allow targets to decide whether such bihacks are actually profitable, or it's best to keep them as selects.

In D63382#1707153, @spatel wrote:

@joanlluch - this is a case where we could transform incoming shift to select and benefit small targets as you've noted.

It works for me.

This fixed bug 42528. okey to me.

@craig.topper , can this be considered ok for the X86?. Thanks.

Hi Craig, thanks for commenting.
Yes, I was actually compiling for -Oz, but the differences when using -Os are even bigger. Let me try to explain every case.

• For Oz, the compiler indeed generates expensive push/pop instructions as an attempt to reduce code size. However, after the patch is applied these instructions are removed without any code size increase. I have checked that in several scenarios and the result is either the same size or less size. This is because the patch reduces the overall number of instructions. On the examples above, the resulting code size is identical, this is because the sequence:

Hi shchenz,

Fixes reported assertion crash on SystemZ

I recently sent a new LSR patch proposal, D63692, that to some extent runs in the opposite direction than this one. I mean, I am adding an additional initial formula for ICmpZero as opposed to excluding it. I think that both patches are however fully compatible and probably worth merging together to get the best of all platforms. I added @shchenz, the author of this, as a D63692 reviewer so he can post any considerations.