[AMDGPU] Fix ds_read2/write2 with unaligned offsets

[foad]

Summary

These instructions use a scaled offset. We were wrongly selecting them
even when the required offset was not a multiple of the scale factor.

[foad]

See also D86627.

[arsenm]

Does the GlobalISel pattern have the same problem?

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1262

I think the variable name align here is misleading since this isn't the memory alignment

1270–1272

split this into a isDSOffset2Legal function?

[foad]

Use new isDSOffset2Legal, do the same for globalisel, and fix a globalisel bug with large offsets.

[foad]

In D90607#2368689, @arsenm wrote:

Does the GlobalISel pattern have the same problem?

No, it had a different one :) For read2/write2_b64 it was passing 16 in as the last argument to isDSOffsetLegal, which is the bit width of the "offset" fields in the instruction. I think it's all good now.

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1262

I used Size for the size in bytes of each of the 2 accesses.

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

bool AMDGPUDAGToDAGISel::SelectDS128Bit8ByteAligned(SDValue Addr, SDValue &Base,
                                                     SDValue &Offset1) const {
   return SelectDSReadWrite2(Addr, Base, Offset0, Offset1, true);
 }
 
 bool AMDGPUDAGToDAGISel::SelectDSReadWrite2(SDValue Addr, SDValue &Base,
                                             SDValue &Offset0, SDValue &Offset1,
                                             bool IsDS128) const {
   SDLoc DL(Addr);
   unsigned Align = IsDS128 ? 8 : 4;
 
   if (CurDAG->isBaseWithConstantOffset(Addr)) {
     SDValue N0 = Addr.getOperand(0);
     SDValue N1 = Addr.getOperand(1);
     ConstantSDNode *C1 = cast<ConstantSDNode>(N1);
     unsigned OffsetValue0 = C1->getZExtValue() / Align;
     unsigned OffsetValue1 = OffsetValue0 + 1;
+    bool OffsetIsAligned = Align * OffsetValue0 == C1->getZExtValue();
     // (add n0, c0)
-    if (isDSOffsetLegal(N0, OffsetValue1, 8)) {
+    if (isDSOffsetLegal(N0, OffsetValue1, 8) && OffsetIsAligned) {
       Base = N0;
       Offset0 = CurDAG->getTargetConstant(OffsetValue0, DL, MVT::i8);
       Offset1 = CurDAG->getTargetConstant(OffsetValue1, DL, MVT::i8);
       return true;
     }
   } else if (Addr.getOpcode() == ISD::SUB) {
     // sub C, x -> add (sub 0, x), C
     if (const ConstantSDNode *C =
             dyn_cast<ConstantSDNode>(Addr.getOperand(0))) {
       unsigned OffsetValue0 = C->getZExtValue() / Align;
       unsigned OffsetValue1 = OffsetValue0 + 1;
+      bool OffsetIsAligned = Align * OffsetValue0 == C->getZExtValue();
 
-      if (isUInt<8>(OffsetValue0)) {
+      if (isUInt<8>(OffsetValue0) && OffsetIsAligned) {
         SDLoc DL(Addr);
         SDValue Zero = CurDAG->getTargetConstant(0, DL, MVT::i32);
 
         // XXX - This is kind of hacky. Create a dummy sub node so we can check
         // the known bits in isDSOffsetLegal. We need to emit the selected node
         // here, so this is thrown away.
         SDValue Sub =
             CurDAG->getNode(ISD::SUB, DL, MVT::i32, Zero, Addr.getOperand(1));

llvm/test/CodeGen/AMDGPU/ds-sub-offset.ll

; GFX9-NEXT:    s_endpgm
   store volatile i32 13, i32 addrspace(3)* %ptr
   ret void
 }
 
 define amdgpu_kernel void @add_x_shl_neg_to_sub_misaligned_i64_max_offset() #1 {
 ; CI-LABEL: add_x_shl_neg_to_sub_misaligned_i64_max_offset:
 ; CI:       ; %bb.0:
 ; CI-NEXT:    v_lshlrev_b32_e32 v0, 2, v0
-; CI-NEXT:    v_sub_i32_e32 v0, vcc, 0, v0
+; CI-NEXT:    v_sub_i32_e32 v0, vcc, 0x3fb, v0
 ; CI-NEXT:    v_mov_b32_e32 v1, 0x7b
 ; CI-NEXT:    v_mov_b32_e32 v2, 0
 ; CI-NEXT:    s_mov_b32 m0, -1
-; CI-NEXT:    ds_write2_b32 v0, v1, v2 offset0:254 offset1:255
+; CI-NEXT:    ds_write2_b32 v0, v1, v2 offset1:1
 ; CI-NEXT:    s_endpgm
 ;
 ; GFX9-LABEL: add_x_shl_neg_to_sub_misaligned_i64_max_offset:
 ; GFX9:       ; %bb.0:
 ; GFX9-NEXT:    v_lshlrev_b32_e32 v0, 2, v0
-; GFX9-NEXT:    v_sub_u32_e32 v0, 0, v0
+; GFX9-NEXT:    v_sub_u32_e32 v0, 0x3fb, v0
 ; GFX9-NEXT:    v_mov_b32_e32 v1, 0x7b
 ; GFX9-NEXT:    v_mov_b32_e32 v2, 0
-; GFX9-NEXT:    ds_write2_b32 v0, v1, v2 offset0:254 offset1:255
+; GFX9-NEXT:    ds_write2_b32 v0, v1, v2 offset1:1
 ; GFX9-NEXT:    s_endpgm
   %x.i = call i32 @llvm.amdgcn.workitem.id.x() #0
   %neg = sub i32 0, %x.i
   %shl = shl i32 %neg, 2
   %add = add i32 1019, %shl
   %ptr = inttoptr i32 %add to i64 addrspace(3)*
   store i64 123, i64 addrspace(3)* %ptr, align 4
   ret void
 }
 
 define amdgpu_kernel void @add_x_shl_neg_to_sub_misaligned_i64_max_offset_clamp_bit(float %dummy.val) #1 {
 ; CI-LABEL: add_x_shl_neg_to_sub_misaligned_i64_max_offset_clamp_bit:
 ; CI:       ; %bb.0:
 ; CI-NEXT:    s_load_dword s0, s[0:1], 0x9
 ; CI-NEXT:    v_lshlrev_b32_e32 v0, 2, v0
-; CI-NEXT:    v_sub_i32_e32 v0, vcc, 0, v0
+; CI-NEXT:    v_sub_i32_e32 v0, vcc, 0x3fb, v0
 ; CI-NEXT:    s_mov_b64 vcc, 0
 ; CI-NEXT:    v_mov_b32_e32 v2, 0x7b
 ; CI-NEXT:    s_waitcnt lgkmcnt(0)
 ; CI-NEXT:    v_mov_b32_e32 v1, s0
 ; CI-NEXT:    s_mov_b32 s0, 0
 ; CI-NEXT:    v_div_fmas_f32 v1, v1, v1, v1
 ; CI-NEXT:    v_mov_b32_e32 v3, 0
 ; CI-NEXT:    s_mov_b32 m0, -1
 ; CI-NEXT:    s_mov_b32 s3, 0xf000
 ; CI-NEXT:    s_mov_b32 s2, -1
 ; CI-NEXT:    s_mov_b32 s1, s0
-; CI-NEXT:    ds_write2_b32 v0, v2, v3 offset0:254 offset1:255
+; CI-NEXT:    ds_write2_b32 v0, v2, v3 offset1:1
 ; CI-NEXT:    buffer_store_dword v1, off, s[0:3], 0
 ; CI-NEXT:    s_endpgm
 ;
 ; GFX9-LABEL: add_x_shl_neg_to_sub_misaligned_i64_max_offset_clamp_bit:
 ; GFX9:       ; %bb.0:
 ; GFX9-NEXT:    s_load_dword s0, s[0:1], 0x24
 ; GFX9-NEXT:    s_mov_b64 vcc, 0
 ; GFX9-NEXT:    v_lshlrev_b32_e32 v0, 2, v0
-; GFX9-NEXT:    v_sub_u32_e32 v0, 0, v0
+; GFX9-NEXT:    v_sub_u32_e32 v0, 0x3fb, v0
 ; GFX9-NEXT:    v_mov_b32_e32 v3, 0
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX9-NEXT:    v_mov_b32_e32 v1, s0
 ; GFX9-NEXT:    v_div_fmas_f32 v2, v1, v1, v1
 ; GFX9-NEXT:    v_mov_b32_e32 v1, 0x7b
-; GFX9-NEXT:    ds_write2_b32 v0, v1, v3 offset0:254 offset1:255
+; GFX9-NEXT:    ds_write2_b32 v0, v1, v3 offset1:1
 ; GFX9-NEXT:    v_mov_b32_e32 v0, 0
 ; GFX9-NEXT:    v_mov_b32_e32 v1, 0
 ; GFX9-NEXT:    global_store_dword v[0:1], v2, off
 ; GFX9-NEXT:    s_endpgm
   %x.i = call i32 @llvm.amdgcn.workitem.id.x() #0
   %neg = sub i32 0, %x.i
   %shl = shl i32 %neg, 2
   %add = add i32 1019, %shl

llvm/test/CodeGen/AMDGPU/ds_read2.ll

 ; GFX9-ALIGNED-NEXT:    s_endpgm
 ;
 ; GFX9-UNALIGNED-LABEL: unaligned_offset_read2_f32:
 ; GFX9-UNALIGNED:       ; %bb.0:
 ; GFX9-UNALIGNED-NEXT:    s_load_dwordx2 s[2:3], s[0:1], 0x24
 ; GFX9-UNALIGNED-NEXT:    s_load_dword s0, s[0:1], 0x2c
 ; GFX9-UNALIGNED-NEXT:    v_lshlrev_b32_e32 v2, 2, v0
 ; GFX9-UNALIGNED-NEXT:    s_waitcnt lgkmcnt(0)
-; GFX9-UNALIGNED-NEXT:    v_add_u32_e32 v0, s0, v2
-; GFX9-UNALIGNED-NEXT:    ds_read2_b32 v[0:1], v0 offset0:1 offset1:2
+; GFX9-UNALIGNED-NEXT:    v_add3_u32 v0, s0, v2, 5
+; GFX9-UNALIGNED-NEXT:    ds_read2_b32 v[0:1], v0 offset1:1
 ; GFX9-UNALIGNED-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX9-UNALIGNED-NEXT:    v_add_f32_e32 v0, v0, v1
 ; GFX9-UNALIGNED-NEXT:    global_store_dword v2, v0, s[2:3]
 ; GFX9-UNALIGNED-NEXT:    s_endpgm
   %x.i = tail call i32 @llvm.amdgcn.workitem.id.x() #1
   %base = getelementptr inbounds float, float addrspace(3)* %lds, i32 %x.i
   %base.i8 = bitcast float addrspace(3)* %base to i8 addrspace(3)*
   %addr0.i8 = getelementptr inbounds i8, i8 addrspace(3)* %base.i8, i32 5

llvm/test/CodeGen/AMDGPU/ds_write2.ll

 ; GFX9-UNALIGNED-LABEL: unaligned_offset_simple_write2_one_val_f64:
 ; GFX9-UNALIGNED:       ; %bb.0:
 ; GFX9-UNALIGNED-NEXT:    s_load_dwordx2 s[2:3], s[0:1], 0x2c
 ; GFX9-UNALIGNED-NEXT:    s_load_dword s0, s[0:1], 0x34
 ; GFX9-UNALIGNED-NEXT:    v_lshlrev_b32_e32 v2, 3, v0
 ; GFX9-UNALIGNED-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX9-UNALIGNED-NEXT:    global_load_dwordx2 v[0:1], v2, s[2:3]
 ; GFX9-UNALIGNED-NEXT:    v_add_u32_e32 v2, s0, v2
+; GFX9-UNALIGNED-NEXT:    v_add_u32_e32 v3, 5, v2
+; GFX9-UNALIGNED-NEXT:    v_add_u32_e32 v2, 9, v2
 ; GFX9-UNALIGNED-NEXT:    s_waitcnt vmcnt(0)
-; GFX9-UNALIGNED-NEXT:    ds_write2_b32 v2, v0, v1 offset0:1 offset1:2
-; GFX9-UNALIGNED-NEXT:    ds_write2_b32 v2, v0, v1 offset0:2 offset1:3
+; GFX9-UNALIGNED-NEXT:    ds_write2_b32 v3, v0, v1 offset1:1
+; GFX9-UNALIGNED-NEXT:    ds_write2_b32 v2, v0, v1 offset1:1
 ; GFX9-UNALIGNED-NEXT:    s_endpgm
   %x.i = tail call i32 @llvm.amdgcn.workitem.id.x() #1
   %in.gep = getelementptr double, double addrspace(1)* %in, i32 %x.i
   %val = load double, double addrspace(1)* %in.gep, align 8
   %base = getelementptr inbounds double, double addrspace(3)* %lds, i32 %x.i
   %base.i8 = bitcast double addrspace(3)* %base to i8 addrspace(3)*
   %addr0.i8 = getelementptr inbounds i8, i8 addrspace(3)* %base.i8, i32 5
   %addr0 = bitcast i8 addrspace(3)* %addr0.i8 to double addrspace(3)*