[AMDGPU] Refactor ds_read/ds_write related select code for better readability.

[hsmhsm]

Summary

Part of the code related to ds_read/ds_write ISel is refactored, and the
corresponding comment is re-written for better readability, which would help
while implementing any future ds_read/ds_write ISel related modifications.

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

1415–1416

"related hardware bug" - no idea what this is referring to

[hsmhsm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

1415–1416

This is actually referring to - https://github.com/llvm/llvm-project/blob/6c0a1ed3a94ff34e6d9500cdfd04858b1a6f72aa/llvm/lib/Target/AMDGPU/AMDGPU.td#L175

The boolean function hasLDSMisalignedBug() is used to track it.

[rampitec]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

1431–1433

It drops the comment about gfx8 and older which is probably still relevant.

1439–1440

Same here.

[hsmhsm]

Fixed review comments by Stas.

[hsmhsm]

PING

[foad]

llvm/lib/Target/AMDGPU/DSInstructions.td

843–847

Please just merge these two foreach loops. Same for the other pairs of foreach loops which are now adjacent.

880

I think something like "do not select ds_read_b128/ds_write_b128 for unaligned accesses" would be more accurate. We still select them for aligned accesses.

Also "at lower alignments" doesn't make much sense now you have changed the first half of the comment not to mention any particular alignment.

llvm/lib/Target/AMDGPU/DSInstructions.td

 
 foreach vt = Reg32Types.types in {
 defm : DSReadPat_mc <DS_READ_B32, vt, "load_local">;
 }
 
 defm : DSReadPat_mc <DS_READ_B32, i32, "atomic_load_32_local">;
 defm : DSReadPat_mc <DS_READ_B64, i64, "atomic_load_64_local">;
 
-// Prefer ds_read over ds_read2, all other things being equal, because it has
-// a larger immediate offset range.
-let AddedComplexity = 100 in {
-
-foreach vt = VReg_64.RegTypes in {
-defm : DSReadPat_mc <DS_READ_B64, vt, "load_align8_local">;
-}
-
-let SubtargetPredicate = isGFX7Plus in {
-
-foreach vt = VReg_96.RegTypes in {
-defm : DSReadPat_mc <DS_READ_B96, vt, "load_align16_local">;
-}
-
-// For performance reasons restrict this to alignment >= 16 even with
-// unaligned-access-mode. At lower alignments ds_read2_b64 is always a better
-// choice.
-foreach vt = VReg_128.RegTypes in {
-defm : DSReadPat_mc <DS_READ_B128, vt, "load_align16_local">;
-}
-
-let SubtargetPredicate = HasUnalignedAccessMode in {
-
-foreach vt = VReg_96.RegTypes in {
-defm : DSReadPat_mc <DS_READ_B96, vt, "load_local">;
-}
-
-} // End SubtargetPredicate = HasUnalignedAccessMode
-
-} // End SubtargetPredicate = isGFX7Plus
-
-} // End AddedComplexity = 100
-
 let OtherPredicates = [D16PreservesUnusedBits] in {
 def : DSReadPat_D16<DS_READ_U16_D16_HI, load_d16_hi_local, v2i16>;
 def : DSReadPat_D16<DS_READ_U16_D16_HI, load_d16_hi_local, v2f16>;
 def : DSReadPat_D16<DS_READ_U8_D16_HI, az_extloadi8_d16_hi_local, v2i16>;
 def : DSReadPat_D16<DS_READ_U8_D16_HI, az_extloadi8_d16_hi_local, v2f16>;
 def : DSReadPat_D16<DS_READ_I8_D16_HI, sextloadi8_d16_hi_local, v2i16>;
 def : DSReadPat_D16<DS_READ_I8_D16_HI, sextloadi8_d16_hi_local, v2f16>;
 

 foreach vt = VReg_64.RegTypes in {
 defm : DS64Bit4ByteAlignedPat_mc<vt>;
 }
 
 foreach vt = VReg_128.RegTypes in {
 defm : DS128Bit8ByteAlignedPat_mc<vt>;
 }
 
-// Prefer ds_write over ds_write2, all other things being equal, because it has
-// a larger immediate offset range.
+// Prefer ds_read over ds_read2 and ds_write over ds_write2, all other things
+// being equal, because it has a larger immediate offset range.
 let AddedComplexity = 100 in {
 
 foreach vt = VReg_64.RegTypes in {
+defm : DSReadPat_mc <DS_READ_B64, vt, "load_align8_local">;
+}
+
+foreach vt = VReg_64.RegTypes in {
 defm : DSWritePat_mc <DS_WRITE_B64, vt, "store_align8_local">;
 }
 
 let SubtargetPredicate = isGFX7Plus in {
 
 foreach vt = VReg_96.RegTypes in {
+defm : DSReadPat_mc <DS_READ_B96, vt, "load_align16_local">;
+}
+
+foreach vt = VReg_96.RegTypes in {
 defm : DSWritePat_mc <DS_WRITE_B96, vt, "store_align16_local">;
 }
 
-// For performance reasons restrict this to alignment >= 16 even with
-// unaligned-access-mode. At lower alignments ds_write2_b64 is always a better
-// choice.
+foreach vt = VReg_128.RegTypes in {
+defm : DSReadPat_mc <DS_READ_B128, vt, "load_align16_local">;
+}
+
 foreach vt = VReg_128.RegTypes in {
 defm : DSWritePat_mc <DS_WRITE_B128, vt, "store_align16_local">;
 }
 
 let SubtargetPredicate = HasUnalignedAccessMode in {
 
+// FIXME: Is ds_read_b96/ds_write_b96 better choice in unaligned-access-mode?
+foreach vt = VReg_96.RegTypes in {
+defm : DSReadPat_mc <DS_READ_B96, vt, "load_local">;
+}
+
 foreach vt = VReg_96.RegTypes in {
 defm : DSWritePat_mc <DS_WRITE_B96, vt, "store_local">;
 }
 
+// For performance reasons, *do not* select ds_read_b128/ds_write_b128 in
+// unaligned-access-mode. At lower alignments ds_read2_b64/ds_write2_b64 is
+// always a better choice.
+
 } // End SubtargetPredicate = HasUnalignedAccessMode
 
 } // End SubtargetPredicate = isGFX7Plus
 
 } // End AddedComplexity = 100
 
 class DSAtomicRetPat<DS_Pseudo inst, ValueType vt, PatFrag frag, bit gds=0> : GCNPat <
   (frag (DS1Addr1Offset i32:$ptr, i16:$offset), vt:$value),

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

if (AddrSpace == AMDGPUAS::LOCAL_ADDRESS ||
     // disabled.
     if (Subtarget->hasUnalignedDSAccessEnabled() &&
         !Subtarget->hasLDSMisalignedBug()) {
       if (IsFast)
         *IsFast = Alignment != Align(2);
       return true;
     }
 
+    // Either, the alignment requirements are "enabled", or there is an
+    // unaligned LDS access related hardware bug though alignment requirements
+    // are "disabled". In either case, we need to check for proper alignment
+    // requirements.
+    //
     if (Size == 64) {
-      // ds_read/write_b64 require 8-byte alignment, but we can do a 4 byte
-      // aligned, 8 byte access in a single operation using ds_read2/write2_b32
-      // with adjacent offsets.
+      // 8 byte accessing via ds_read/write_b64 require 8-byte alignment, but we
+      // can do a 4 byte aligned, 8 byte access in a single operation using
+      // ds_read2/write2_b32 with adjacent offsets.
       bool AlignedBy4 = Alignment >= Align(4);
       if (IsFast)
         *IsFast = AlignedBy4;
 
       return AlignedBy4;
     }
     if (Size == 96) {
-      // ds_read/write_b96 require 16-byte alignment on gfx8 and older.
-      bool Aligned = Alignment >= Align(16);
+      // 12 byte accessing via ds_read/write_b96 require 16-byte alignment on
+      // gfx8 and older.
+      bool AlignedBy16 = Alignment >= Align(16);
       if (IsFast)
-        *IsFast = Aligned;
+        *IsFast = AlignedBy16;
 
-      return Aligned;
+      return AlignedBy16;
     }
     if (Size == 128) {
-      // ds_read/write_b128 require 16-byte alignment on gfx8 and older, but we
-      // can do a 8 byte aligned, 16 byte access in a single operation using
-      // ds_read2/write2_b64.
-      bool Aligned = Alignment >= Align(8);
+      // 16 byte accessing via ds_read/write_b128 require 16-byte alignment on
+      // gfx8 and older, but  we can do a 8 byte aligned, 16 byte access in a
+      // single operation using ds_read2/write2_b64.
+      bool AlignedBy8 = Alignment >= Align(8);
       if (IsFast)
-        *IsFast = Aligned;
+        *IsFast = AlignedBy8;
 
-      return Aligned;
+      return AlignedBy8;
     }
   }
 
   if (AddrSpace == AMDGPUAS::PRIVATE_ADDRESS) {
     bool AlignedBy4 = Alignment >= Align(4);
     if (IsFast)
       *IsFast = AlignedBy4;