Page MenuHomePhabricator

[DAGCombiner] Transform (zext (select c, load1, load2)) -> (select c, zextload1, zextload2)
ClosedPublic

Authored by Carrot on Jan 20 2021, 1:14 PM.

Details

Summary

If extload is legal, following transform

(zext (select c, load1, load2)) -> (select c, zextload1, zextload2)

can save one ext instruction.

Diff Detail

Event Timeline

Carrot created this revision.Jan 20 2021, 1:14 PM
Carrot requested review of this revision.Jan 20 2021, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 1:14 PM

Can you precommit testcase, please?

Carrot updated this revision to Diff 318305.Jan 21 2021, 1:38 PM

Rebase the code.

RKSimon added inline comments.Jan 26 2021, 2:41 PM
llvm/test/CodeGen/X86/select-ext.ll
73–146

Worth adding sextload test case? What about vectors?

Carrot added inline comments.Jan 26 2021, 6:15 PM
llvm/test/CodeGen/X86/select-ext.ll
73–146

Never thought about vector cases. Will look into it.

Carrot updated this revision to Diff 319723.Jan 27 2021, 5:33 PM

Add more test cases.

llvm/test/CodeGen/X86/select-ext.ll
73–146

The current patch can also handle vectors. In the new test case, sext_vector is compiled to following without this patch

sext_vector:                            # @sext_vector
        .cfi_startproc
# %bb.0:
        movq    (%rdi), %xmm1                   # xmm1 = mem[0],zero
        movq    8(%rdi), %xmm0                  # xmm0 = mem[0],zero
        testl   %esi, %esi
        jne     .LBB3_2
# %bb.1:
        movdqa  %xmm1, %xmm0
.LBB3_2:
        pmovsxdq        %xmm0, %xmm0
        retq

With this patch I got

sext_vector:                            # @sext_vector
        .cfi_startproc
# %bb.0:
        pmovsxdq        (%rdi), %xmm1
        pmovsxdq        8(%rdi), %xmm0
        testl   %esi, %esi
        jne     .LBB3_2
# %bb.1:
        movdqa  %xmm1, %xmm0
.LBB3_2:
        retq

The sign extension instruction before return is removed.

@Carrot I've added the new tests to trunk along with some vselect test cases - please can you rebase?

Carrot updated this revision to Diff 319914.Jan 28 2021, 10:20 AM

Rebase the test case.

RKSimon added inline comments.Jan 28 2021, 2:30 PM
llvm/test/CodeGen/X86/select-ext.ll
106

Any chance we can support ISD::VSELECT as well?

Carrot updated this revision to Diff 320011.Jan 28 2021, 5:21 PM
Carrot marked an inline comment as done.

Add support for ISD::VSELECT.

xbolva00 added inline comments.Jan 28 2021, 5:26 PM
llvm/test/CodeGen/X86/select-ext.ll
57

Remove todo

Carrot updated this revision to Diff 320018.Jan 28 2021, 6:38 PM
Carrot marked an inline comment as done.
RKSimon added inline comments.Jan 29 2021, 3:19 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10057

We also need to ensure that the loads aren't already a extload - or are either the same type of extension (or just ISD::EXTLOAD which I guess we can replace). This will need more tests (inc negative tests).

Carrot updated this revision to Diff 320211.Fri, Jan 29, 1:51 PM
Carrot marked an inline comment as done.

Check conflicting extload.

RKSimon added inline comments.Tue, Feb 9, 7:55 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10097

I'm not sure if this correctly handles the case where Opcode is ANY_EXTEND and the existing loads are SEXT/ZEXT - in which case we need to ensure the final (legal) loads are fully SEXT/ZEXT loads - even though we don't care about the new extended bits we do still need the 'middle' extended bits to be correct.

Does that make sense? I'm not certain if this is handled - I may have gotten this wrong and haven't gotten a test case for this.

Carrot updated this revision to Diff 322847.Wed, Feb 10, 3:52 PM

ANY_EXTEND of SEXTLOAD/ZEXTLOAD should not be allowed.
In theory if both loads have same SEXTLOAD/ZEXTLOAD, we can change ExpOpcode to corresponding SIGN_EXTEND/ZERO_EXTEND, but it will make the code more complex, and this case should be extremely rare.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10097

You are right, ANY_EXTEND of SEXTLOAD/ZEXTLOAD was not correctly handled.

Does anyone else have any comments?

RKSimon accepted this revision.Wed, Feb 17, 6:35 AM

LGTM - cheers

llvm/test/CodeGen/X86/select-ext.ll
91

Please pre-commit these additional tests against trunk then update with this patch so the commit shows the diff/nodiff.

This revision is now accepted and ready to land.Wed, Feb 17, 6:35 AM