[PATCH] D76201: [TargetLowering] Only demand a rotation's modulo amount bits

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/test/CodeGen/SystemZ/rot-01.ll:12
 ; CHECK-NEXT:    br %r14
   %mod = urem i32 %amt, 32
 
----------------
In order for this test to still test what it is intended to test (see below), this should probably be changed to 16 instead of 32 now.


================
Comment at: llvm/test/CodeGen/SystemZ/rot-01.ll:30
 ; CHECK-NEXT:    br %r14
   %mod = urem i64 %amt, 32
 
----------------
RKSimon wrote:
> @uweigand Should this be 32 or 64?
This should be 32.  The point of this test is that we actually need the AND, but we should use the most efficient instruction (when using the result of the AND only in a rotate, we can implement the AND via NILL instead of the NILF we'd otherwise require).


================
Comment at: llvm/test/CodeGen/SystemZ/rot-02.ll:7
 
 ; Test that AND is not removed when some lower 6 bits are not set.
 define i32 @f1(i32 %val, i32 %amt) {
----------------
RKSimon wrote:
> @uweigand This comment doesn't seem to match the test - any suggestions?
With these changes, only 5 bits count (instead of 6 bits) when operating on 32-bit variables.  You should probably change the comment here to 5 bits, and then change the AND constant from 31 to 15 or so.


================
Comment at: llvm/test/CodeGen/SystemZ/shift-04.ll:101
 ; Check shift amounts that have the largest in-range constant term.  We could
 ; mask the amount instead.
 define i32 @f8(i32 %a, i32 %amt) {
----------------
Now that we actually do mask the amount, the comment should be updated.


================
Comment at: llvm/test/CodeGen/SystemZ/shift-04.ll:120
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    afi %r3, 524288
 ; CHECK-NEXT:    rll %r2, %r2, 0(%r3)
----------------
Now I'm wondering: why doesn't the masking also work here?


================
Comment at: llvm/test/CodeGen/SystemZ/shift-04.ll:150
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    rll %r2, %r2, -524288(%r3)
 ; CHECK-NEXT:    br %r14
----------------
Or here?


================
Comment at: llvm/test/CodeGen/SystemZ/shift-04.ll:161
 ; Check the next value down, which without masking must use a separate
 ; addition.
 define i32 @f12(i32 %a, i32 %amt) {
----------------
Likewise.


================
Comment at: llvm/test/CodeGen/SystemZ/shift-08.ll:102
 ; Check shift amounts that have the largest in-range constant term.  We could
 ; mask the amount instead.
 define i64 @f8(i64 %a, i64 %amt) {
----------------
Same comments as with shift-04.ll apply to this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76201