[DAGCombine] Fix bug in load scalarization

[frasercrmck]

Summary

For vector element types of less than one byte in size, we would
generate incorrect scalar offsets and produce incorrect codegen.

This optimization could be supported in the future, e.g. by loading a
byte, then shifting and masking out the smaller vector element type.
However, without an upstream target to test against it's best to avoid
the bad codegen in the simplest possible way.

Related to this bug:

https://bugs.llvm.org/show_bug.cgi?id=27600

[lebedev.ri]

Tests missing

[frasercrmck]

In D78568#1994885, @lebedev.ri wrote:

Tests missing

I'll see what I can do. We caught it on our downstream target so I'll have to find an appropriate upstream one. Do you have any suggestions for targets to start with?

[frasercrmck]

Add a test case

[foad]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

17786

I think using .isByteSized would be more obvious and more correct, since it would catch cases like i12 which also can't be handled by indexing.

[frasercrmck]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

17786

Yes, that's a good point

(sigh: just realised that I didn't click "submit" on this one)

[frasercrmck]

Extend the fix to any non-byte-sized vector element

[foad]

Looks good to me. You could pre-commit the test case and rebase this diff so we can see the effect on codegen.

llvm/test/CodeGen/AMDGPU/extract_load_i1.ll

1

Generate the checks with utils/update_llc_test_checks.py? It looks like you might have done that already, except that it should leave a comment on line 1.

[frasercrmck]

In D78568#2349707, @foad wrote:

Looks good to me. You could pre-commit the test case and rebase this diff so we can see the effect on codegen.

Sure, I can do that. Is committing a test that "expects" incorrect codegen the done thing?

[foad]

In D78568#2350291, @frasercrmck wrote:

In D78568#2349707, @foad wrote:

Looks good to me. You could pre-commit the test case and rebase this diff so we can see the effect on codegen.

Sure, I can do that. Is committing a test that "expects" incorrect codegen the done thing?

Yes, especially if it's only temporary and the checks are generated.

[frasercrmck]

In D78568#2350310, @foad wrote:

In D78568#2350291, @frasercrmck wrote:

In D78568#2349707, @foad wrote:

Looks good to me. You could pre-commit the test case and rebase this diff so we can see the effect on codegen.

Sure, I can do that. Is committing a test that "expects" incorrect codegen the done thing?

Yes, especially if it's only temporary and the checks are generated.

Okay, great. The test is currently generated as:

define i1 @extractloadi1(<8 x i1> *%ptr, i32 %idx) {
; CHECK-LABEL: extractloadi1:
; CHECK:       ; %bb.0:
; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; CHECK-NEXT:    v_mad_u64_u32 v[0:1], s[4:5], v2, 1, v[0:1]
; CHECK-NEXT:    flat_load_ubyte v0, v[0:1]
; CHECK-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
; CHECK-NEXT:    s_setpc_b64 s[30:31]
  %val = load <8 x i1>, <8 x i1> *%ptr
  %ret = extractelement <8 x i1> %val, i32 %idx
  ret i1 %ret
}

I believe this shows the incorrect codegen.

[frasercrmck]

Update test to show fixed codegen, and test renamed from underscores to hyphens.

[frasercrmck]

@foad did you want to look over how codegen is affected by this fix before approving?

[foad]

In D78568#2373924, @frasercrmck wrote:

@foad did you want to look over how codegen is affected by this fix before approving?

It looks fine to me. Actually it looks very poor, but that's not your fault... :)

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 
 SDValue DAGCombiner::scalarizeExtractedVectorLoad(SDNode *EVE, EVT InVecVT,
                                                   SDValue EltNo,
                                                   LoadSDNode *OriginalLoad) {
   assert(OriginalLoad->isSimple());
 
   EVT ResultVT = EVE->getValueType(0);
   EVT VecEltVT = InVecVT.getVectorElementType();
+
+  // If the vector element type is not a multiple of a byte then we are unable
+  // to correctly compute an address to load only the extracted element as a
+  // scalar.
+  if (!VecEltVT.isByteSized())
+    return SDValue();
+
   Align Alignment = OriginalLoad->getAlign();
   Align NewAlign = DAG.getDataLayout().getABITypeAlign(
       VecEltVT.getTypeForEVT(*DAG.getContext()));
 
   if (NewAlign > Alignment ||
       !TLI.isOperationLegalOrCustom(ISD::LOAD, VecEltVT))
     return SDValue();
 

llvm/test/CodeGen/AMDGPU/extract_load_i1.ll

+; RUN: llc -mtriple=amdgcn-amd-amdhsa < %s | FileCheck %s
+
+; FIXME: This could be much better
+; CHECK-LABEL: extractloadi1:
+; CHECK: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK: flat_load_ubyte v0, v[0:1]
+; CHECK: v_and_b32_e32 v1, 7, v2
+; CHECK: v_lshr_b32_e64 v9, s32, 6
+; CHECK: v_or_b32_e32 v1, v9, v1
+; CHECK: s_waitcnt vmcnt(0) lgkmcnt(0)
+; CHECK: v_bfe_u32 v2, v0, 1, 1
+; CHECK: v_bfe_u32 v3, v0, 2, 2
+; CHECK: v_bfe_u32 v4, v0, 3, 1
+; CHECK: v_lshrrev_b32_e32 v5, 4, v0
+; CHECK: v_bfe_u32 v6, v0, 5, 1
+; CHECK: v_lshrrev_b32_e32 v7, 6, v0
+; CHECK: v_lshrrev_b32_e32 v8, 7, v0
+; CHECK: buffer_store_byte v0, off, s[0:3], s32
+; CHECK: buffer_store_byte v8, off, s[0:3], s32 offset:7
+; CHECK: buffer_store_byte v7, off, s[0:3], s32 offset:6
+; CHECK: buffer_store_byte v6, off, s[0:3], s32 offset:5
+; CHECK: buffer_store_byte v5, off, s[0:3], s32 offset:4
+; CHECK: buffer_store_byte v4, off, s[0:3], s32 offset:3
+; CHECK: buffer_store_byte v3, off, s[0:3], s32 offset:2
+; CHECK: buffer_store_byte v2, off, s[0:3], s32 offset:1
+; CHECK: buffer_load_ubyte v0, v1, s[0:3], 0 offen
+define i1 @extractloadi1(<8 x i1> *%ptr, i32 %idx) {
+  %val = load <8 x i1>, <8 x i1> *%ptr
+  %ret = extractelement <8 x i1> %val, i32 %idx
+  ret i1 %ret
+}