[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

17241

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

17241

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 ↗

(On Diff #300207)

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 narrower than a byte then we are unable to
+  // compute an address to load only the extracted element as a scalar.
+  if (VecEltVT.getSizeInBits() < 8)
+    return SDValue();
+
   unsigned Align = OriginalLoad->getAlignment();
   unsigned NewAlign = DAG.getDataLayout().getABITypeAlignment(
       VecEltVT.getTypeForEVT(*DAG.getContext()));
 
   if (NewAlign > Align || !TLI.isOperationLegalOrCustom(ISD::LOAD, VecEltVT))
     return SDValue();
 
   ISD::LoadExtType ExtTy = ResultVT.bitsGT(VecEltVT) ?