This is an archive of the discontinued LLVM Phabricator instance.

[X86][CodeGen][NFC] Delay `combineIncDecVector()` from DAGCombine to X86DAGToDAGISel
ClosedPublic

Authored by lebedev.ri on May 23 2019, 9:04 AM.

Details

Summary

We were previously doing it in DAGCombine.
But we also want to do sub %x, C -> add %x, (sub 0, C) for vectors in DAGCombine.
So if we had sub %x, -1, we'll transform it to add %x, 1,
which combineIncDecVector() will immediately transform back into sub %x, -1,
and here we go again...

I've marked this as NFC since not a single test changes,
but since that 'changes' DAGCombine, probably this isn't fully NFC.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.May 23 2019, 1:35 PM
lib/Target/X86/X86ISelDAGToDAG.cpp
3449 ↗(On Diff #200991)

What happens to the constant pool value we already allocated. Does something recognize that is unused and not emit to the final assembly?

Could we use a isrematerializable test instead in the (sub x, c) -> (add x, -c) folds to avoid needing this?

lebedev.ri marked 2 inline comments as done.

Rebased, NFC.
@craig.topper yes, constant pool entry is being dropped, at least it isn't present in final .s

lebedev.ri added inline comments.Aug 27 2019, 3:32 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
3449 ↗(On Diff #200991)

I believe so:

$ cat /tmp/test.ll 
define <16 x i8> @inc(<16 x i8> %x) {
  %r = add <16 x i8> %x, <i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1, i8 1>
  ret <16 x i8> %r
}
$ ./bin/llc -o - /tmp/test.ll 
        .text
        .file   "test.ll"
        .globl  inc                     # -- Begin function inc
        .p2align        4, 0x90
        .type   inc,@function
inc:                                    # @inc
        .cfi_startproc
# %bb.0:
        pcmpeqd %xmm1, %xmm1
        psubb   %xmm1, %xmm0
        retq
.Lfunc_end0:
        .size   inc, .Lfunc_end0-inc
        .cfi_endproc
                                        # -- End function

        .section        ".note.GNU-stack","",@progbits
RKSimon added inline comments.Aug 27 2019, 5:37 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
3802 ↗(On Diff #217351)

(style) drop the braces?

llvm/lib/Target/X86/X86ISelLowering.h
686 ↗(On Diff #217351)

Add description comment

lebedev.ri marked 2 inline comments as done.

Thanks for taking a look!
Addressed review notes.

This revision is now accepted and ready to land.Aug 27 2019, 8:17 AM

LGTM

Thank you for the review.

This revision was automatically updated to reflect the committed changes.