This is an archive of the discontinued LLVM Phabricator instance.

[aarch64] move custom isel of extract_vector_elt to td file - NFC
ClosedPublic

Authored by sebpop on Sep 12 2019, 7:13 AM.

Details

Summary

In preparation for def-pat selection of dot product instructions,
this patch moves the custom instruction selection of extract_vector_elt
to the td file. Without this change it is impossible to catch a pattern that
starts with an extract_vector_elt: the custom cpp code is executed first
ahead of the patterns in the td files that are only executed at the end of
the switch statement in SelectCode(Node).

With this patch applied, it becomes possible to select a different pattern
that starts with extract_vector_elt by selecting a higher complexity than
this pattern.

The patch has been tested on aarch64-linux with make check-all.

Diff Detail

Event Timeline

sebpop created this revision.Sep 12 2019, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 7:13 AM
SjoerdMeijer added inline comments.Sep 12 2019, 7:36 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
6978

We were curious why you need a new AArch64 specific DAG node? Can you not use extractelt in the match patterns?

sebpop marked an inline comment as done.Sep 12 2019, 7:49 AM
sebpop added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.td
6978

I did that because two other targets do that: git grep says

SystemZ/SystemZOperators.td:def z_vector_extract    : SDNode<"ISD::EXTRACT_VECTOR_ELT",
X86/X86InstrAVX512.td:def X86kextract : SDNode<"ISD::EXTRACT_VECTOR_ELT",

I tried replacing kextract with extractelt and I got this error:

Type set is empty for each HW mode:
possible type contradiction in the pattern below (use -print-records with llvm-tblgen to see all expanded records).
vtInt: 	(vt:{ *:[Other] })

I still need to see how to solve this error.

Is there a test case that checks that this change does not break what the code in AArch64DAGToDAGISel::Select() was meant to handle?

SjoerdMeijer added inline comments.Sep 12 2019, 8:03 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
6978

Okay, now I see where the name kextract comes from :-)
Before wondering why we need another node, I was wondering what it could mean, and wanted to add that as a nitpick.

That useless tablegen error doesn't ring a bell.... I mean, I have seen it before and then the type contradiction was obvious, but in this case I am a bit clueless.

Is there a test case that checks that this change does not break what the code in AArch64DAGToDAGISel::Select() was meant to handle?

Yes, there are several tests relying on this behavior.
If I just comment out the patterns to the TD file, I get all these tests failing:

Failing Tests (28):
    LLVM :: CodeGen/AArch64/arm64-2012-05-07-DAGCombineVectorExtract.ll
    LLVM :: CodeGen/AArch64/arm64-AdvSIMD-Scalar.ll
    LLVM :: CodeGen/AArch64/arm64-crypto.ll
    LLVM :: CodeGen/AArch64/arm64-neon-copy.ll
    LLVM :: CodeGen/AArch64/arm64-popcnt.ll
    LLVM :: CodeGen/AArch64/arm64-smaxv.ll
    LLVM :: CodeGen/AArch64/arm64-sminv.ll
    LLVM :: CodeGen/AArch64/arm64-vaddv.ll
    LLVM :: CodeGen/AArch64/arm64-vmul.ll
    LLVM :: CodeGen/AArch64/bitcast-v2i8.ll
    LLVM :: CodeGen/AArch64/bitreverse.ll
    LLVM :: CodeGen/AArch64/build-vector-extract.ll
    LLVM :: CodeGen/AArch64/div-rem-pair-recomposition-signed.ll
    LLVM :: CodeGen/AArch64/div-rem-pair-recomposition-unsigned.ll
    LLVM :: CodeGen/AArch64/expand-select.ll
    LLVM :: CodeGen/AArch64/extract-insert.ll
    LLVM :: CodeGen/AArch64/sadd_sat_vec.ll
    LLVM :: CodeGen/AArch64/ssub_sat_vec.ll
    LLVM :: CodeGen/AArch64/trunc-v1i64.ll
    LLVM :: CodeGen/AArch64/uadd_sat_vec.ll
    LLVM :: CodeGen/AArch64/usub_sat_vec.ll
    LLVM :: CodeGen/AArch64/vec_uaddo.ll
    LLVM :: CodeGen/AArch64/vec_umulo.ll
    LLVM :: CodeGen/AArch64/vecreduce-add-legalization.ll
    LLVM :: CodeGen/AArch64/vecreduce-and-legalization.ll
    LLVM :: CodeGen/AArch64/vecreduce-bool.ll
    LLVM :: CodeGen/AArch64/vecreduce-umax-legalization.ll

The tablegen error only happens on the i16 patterns:

def : Pat<(i16 (extractelt (v8i16 V128:$V), (i64 0))), (EXTRACT_SUBREG V128:$V, hsub)>;
def : Pat<(i16 (extractelt (v4i16 V64:$V), (i64 0))), (EXTRACT_SUBREG V64:$V, hsub)>;

If I remove these two patterns, make check-all passes.

I guess the reason for that is because i16s are not legal types so rules producing them would be strange....

sebpop updated this revision to Diff 219930.Sep 12 2019, 9:06 AM

Updated patch by removing the patterns that generate i16. Patch passes make check-all on aarch64-linux.

The patch looks okay to me, but I am still curious what happens with i16. The lowering to umov w8, v0.h[1] in build-vector-extract.ll is probably the interesting one. This is probably covered by rule:

def : Pat<(sext_inreg (vector_extract (v8i16 V128:$Rn), VectorIndexH:$idx),i16),
        (i32 (SMOVvi16to32 V128:$Rn, VectorIndexH:$idx))>;

which is why you don't need the i16 case. But I am not really at my computer, perhaps you can confirm this.

The patch looks okay to me, but I am still curious what happens with i16. The lowering to umov w8, v0.h[1] in build-vector-extract.ll is probably the interesting one. This is probably covered by rule:

The patch would not impact that case as the patch only handles the first lane extraction: "Extracting lane zero is a special case"
The example of umov that you gave is moving the content of the second lane.

def : Pat<(sext_inreg (vector_extract (v8i16 V128:$Rn), VectorIndexH:$idx),i16),
        (i32 (SMOVvi16to32 V128:$Rn, VectorIndexH:$idx))>;

This pattern applies to all lanes.

SjoerdMeijer accepted this revision.Sep 13 2019, 12:21 AM

Ah yes, copy-paste mistake, there is umov extracting the zero element, and you're right about that rule that applies to all lanes.

Anyway, if we are not missing a test case for this, then LGTM.

This revision is now accepted and ready to land.Sep 13 2019, 12:21 AM
This revision was automatically updated to reflect the committed changes.