This is an archive of the discontinued LLVM Phabricator instance.

merge consecutive loads that are offset from a base address (PR21771)
ClosedPublic

Authored by spatel on Dec 12 2014, 9:06 AM.

Details

Summary

SelectionDAG::isConsecutiveLoad() was not detecting consecutive loads when the first load was offset from a base address. This patch recognizes that pattern and subtracts the offset before comparing the second load to see if it is consecutive.

The codegen change in the new test case improves from:

vmovsd	32(%rdi), %xmm0
vmovsd	48(%rdi), %xmm1 
vmovhpd	56(%rdi), %xmm1, %xmm1
vmovhpd	40(%rdi), %xmm0, %xmm0
vinsertf128	$1, %xmm1, %ymm0, %ymm0

To:

vmovups	32(%rdi), %ymm0

An existing test case is also improved from:

vmovsd	(%rdi), %xmm0
vmovsd	16(%rdi), %xmm1
vmovsd	24(%rdi), %xmm2
vunpcklpd	%xmm2, %xmm0, %xmm0 ## xmm0 = xmm0[0],xmm2[0]
vmovhpd	8(%rdi), %xmm1, %xmm3

To:

vmovsd	(%rdi), %xmm0
vmovsd	16(%rdi), %xmm1
vmovhpd	24(%rdi), %xmm0, %xmm0
vmovhpd	8(%rdi), %xmm1, %xmm1

This patch fixes PR21771 ( http://llvm.org/bugs/show_bug.cgi?id=21771 ).

Diff Detail

Event Timeline

spatel updated this revision to Diff 17237.Dec 12 2014, 9:06 AM
spatel retitled this revision from to merge consecutive loads that are offset from a base address (PR21771).
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: hfinkel, andreadb, RKSimon.
spatel added a subscriber: Unknown Object (MLST).
RKSimon edited edge metadata.Dec 13 2014, 5:04 AM

Minor test improvement.

test/CodeGen/X86/chain_order.ll
11

Ideally it'd be better to use CHECK-NEXT - especially as there are more load / float instructions than what is being checked for.

spatel updated this revision to Diff 17283.Dec 15 2014, 7:56 AM
spatel edited edge metadata.

Thanks, Simon! Patch updated with tighter checking of the outputs on the existing test case.

hfinkel accepted this revision.Dec 16 2014, 12:51 PM
hfinkel edited edge metadata.

With the type changes noted below, LGTM.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6558

This is a generic offset, and should be int64_t.

6562

The LLVM convention is to call the type 'int', not 'signed'. Please say (int)Bytes.

6567

Same here, this needs to be int64_t.

6569

Same here.

This revision is now accepted and ready to land.Dec 16 2014, 12:51 PM
This revision was automatically updated to reflect the committed changes.

Thanks, Hal!

Fixed variable type slop and checked in at r224379.