Page MenuHomePhabricator

[DAGCombine] Fix bug in load scalarization
ClosedPublic

Authored by frasercrmck on Apr 21 2020, 8:46 AM.

Details

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

Diff Detail

Event Timeline

frasercrmck created this revision.Apr 21 2020, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 8:46 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

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?

Add a test case

foad added inline comments.Apr 23 2020, 5:16 AM
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 added inline comments.Jun 8 2020, 7:12 AM
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)

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

frasercrmck marked an inline comment as done.Oct 23 2020, 3:02 AM
foad added a comment.Oct 23 2020, 5:01 AM

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.

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 added a comment.Oct 23 2020, 8:29 AM

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.

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.

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

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

foad accepted this revision.Wed, Nov 4, 9:32 AM

@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... :)

This revision is now accepted and ready to land.Wed, Nov 4, 9:32 AM
This revision was landed with ongoing or failed builds.Wed, Nov 4, 11:09 AM
This revision was automatically updated to reflect the committed changes.