[PATCH] D150107: [X86] Remove patterns for shift/rotate with immediate 1 and update side effect

[Kan Shengchen via Phabricator via llvm-commits]

skan marked 3 inline comments as done.
skan added a comment.

In D150107#4328315 <https://reviews.llvm.org/D150107#4328315>, @craig.topper wrote:

> In D150107#4326423 <https://reviews.llvm.org/D150107#4326423>, @skan wrote:
>
>> In D150107#4326328 <https://reviews.llvm.org/D150107#4326328>, @RKSimon wrote:
>>
>>> What is the motivation here?
>>
>> It's first suggested by craig in D150068 <https://reviews.llvm.org/D150068>. I think there are at least three pros
>>
>> 1. This can reduce the patterns during ISEL, as a result, reducing the bytes in X86GenDAGISel.inc
>> 2. The patterns for shift/rotate with immediate 1 look quite similar to shift/rotate with immediate 8. So this can be seen as eliminating "duplicate" code.
>> 3. Delay the optimization from imm8 to imm1, so that the previous optimization passes do not need to handle the version of imm1
>
> Yeah I thought since the code had been moved to a function it was easy to share it and get an isel table reduction.
>
> Looks like it improved fast isel code and made X86DomainReassignment work for shifts by 1. But regressed global isel, though I guess no one should care.

Yes, and thank you for the illustration! The code generated by global isel is far from expected in this test, so I agree no one should care.



================
Comment at: llvm/test/tools/llvm-mca/X86/AlderlakeP/resources-x86_64.s:1518
 # CHECK-NEXT:  3      2     1.00                        rcrb	%dil
-# CHECK-NEXT:  6      13    1.00           *            rclb	(%rax)
-# CHECK-NEXT:  6      13    1.00           *            rcrb	(%rax)
+# CHECK-NEXT:  6      13    1.00    *      *            rclb	(%rax)
+# CHECK-NEXT:  6      13    1.00    *      *            rcrb	(%rax)
----------------
craig.topper wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > I think you need to `let mayLoad = 0`
> > Err nevermind
> This should really be a different patch. This is an existing bug independent of whether removing the patterns gets approved.
I thought so. But when I tried to remove the patterns only in the TD, these tests changed too.  

If I did not add `mayLoad =1,  mayStore=1` explicitly for shift/rotate[m1],  these instructions would show `mayLoad=0, mayStore=0, hasUnModeledSideEffect=1` in these tests.

That's why I updated these bits. It's weird to me if I set `mayLoad=1` for m1 version but set `mayLoad=0` for mi version.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150107/new/

https://reviews.llvm.org/D150107