[GlobalISel] Map extractelt to G_EXTRACT_VECTOR_ELT

[bjope]

Summary

Before this patch there was generic mapping from vector_extract
to G_EXTRACT_VECTOR_ELT added in SelectionDAGCompat.td. That
mapping is now replaced by a mapping from extractelt instead.

The reasoning is that vector_extract is marked as deprecated,
so it is assumed that a majority of targets will use extractelt
and not vector_extract (and that the long term solution for all
targets would be to use extractelt).

Targets like AArch64 that still use vector_extract can add an
additional mapping from the deprecated vector_extract as target
specific tablegen definitions. Such a mapping is added for AArch64
in this patch to avoid breaking tests.

When adding the extractelt => G_EXTRACT_VECTOR_ELT mapping we
triggered some new code paths in GlobalISelEmitter, ending up in
an assert when trying to import a pattern containing EXTRACT_SUBREG
for ARM. Therefore this patch also adds a "failedImport" warning
for that situation (instead of hitting the assert).

[bjope]

Ping!

[dsanders]

Mostly LGTM but I'd rather move the vector_extract back to SelectionDAGCompat.td as having it there doesn't break anything.

llvm/lib/Target/AArch64/AArch64InstrGISel.td

167–168

I'd rather this stay in SelectionDAGCompat.td. Having it there doesn't block backends migrating and doesn't hinder backends that have already migrated, whereas moving it to target specific files breaks any in-tree or downstream target that hasn't migrated yet.

[bjope]

llvm/lib/Target/AArch64/AArch64InstrGISel.td

167–168

I think it will be easier to remove the obsolete ISD nodes if it is obvious that for example only AArch64 depends on it. No in-tree target seem to have broken (or else I'd assume that they should have implemented some lit-tests verifying that vector_exract works).

Although, I'm a bit confused, as your answer in llvm-dev discussion indicated that there could be problems if defining both a relation for vector_extract and one for extractelt for the same target. So I thought it was a bit safer to only have the non-obsolete extractelt in SelectionDAGCompat.td. If this actually do cause problems for AArch64 then I guess it is time to migrate away from using the obsolete ISD node (or we can't have neither mapping in SelectionDAGCompat.td.

[bjope]

llvm/lib/Target/AArch64/AArch64InstrGISel.td

167–168

I should have written "deprecated" instead of "obsolete" in my previous post.

And about the latter part. I did check that AArch64 only use vector_extract. But not having both mappings in SelectionDAGCompat.td could avoid problems for any target that hasn't migrated yet, e.g. if they use both vector_extract and extractelt. They would need to actively do something, adding a mapping for vector_extract, to end up with the potential problem with overlapping rules.

[dsanders]

llvm/lib/Target/AArch64/AArch64InstrGISel.td

167–168

I think it will be easier to remove the obsolete ISD nodes if it is obvious that for example only AArch64 depends on it.

I can see your point there. It's not removable until everyone has removed it and stating which targets haven't migrated is useful. Similarly, it also serves to prevent new targets copying AArch64. On the other hand, I generally prefer to break downstream as little and late as possible. There's no strong case for either approach though. Let's stick with the patch as-is to avoid unnecessary churn.

No in-tree target seem to have broken

That's good news. It's going to break our out-of-tree target but that isn't a reason to not remove the node, it's only a reason to delay a bit at best. It's just unfortunate that holidays meant we're seeing this after the commit instead of getting our fixes in beforehand. We can at least patch it up with the same fix as AArch64 if we need to

Although, I'm a bit confused, as your answer in llvm-dev discussion indicated that there could be problems if defining both a relation for vector_extract and one for extractelt for the same target.

The problem there was having overlapping rules as a result of a target having rules for both vector_extract and extractelt. If you ended up with two rules with the same (or overlapping) pattern but different outputs then there isn't a clear definition for which one will be used (you can manually tie-break with AddedComplexity though). It should be possible to detect that by checking if the pattern is the same as the previous rules pattern but it's likely too rare to be worth checking unless it's already available

llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td

 def : GINodeEquiv<G_BITREVERSE, bitreverse>;
 def : GINodeEquiv<G_FSHL, fshl>;
 def : GINodeEquiv<G_FSHR, fshr>;
 def : GINodeEquiv<G_CTLZ, ctlz>;
 def : GINodeEquiv<G_CTTZ, cttz>;
 def : GINodeEquiv<G_CTLZ_ZERO_UNDEF, ctlz_zero_undef>;
 def : GINodeEquiv<G_CTTZ_ZERO_UNDEF, cttz_zero_undef>;
 def : GINodeEquiv<G_CTPOP, ctpop>;
-def : GINodeEquiv<G_EXTRACT_VECTOR_ELT, vector_extract>;
+def : GINodeEquiv<G_EXTRACT_VECTOR_ELT, extractelt>;
 def : GINodeEquiv<G_CONCAT_VECTORS, concat_vectors>;
 def : GINodeEquiv<G_BUILD_VECTOR, build_vector>;
 def : GINodeEquiv<G_FCEIL, fceil>;
 def : GINodeEquiv<G_FCOS, fcos>;
 def : GINodeEquiv<G_FSIN, fsin>;
 def : GINodeEquiv<G_FABS, fabs>;
 def : GINodeEquiv<G_FSQRT, fsqrt>;
 def : GINodeEquiv<G_FFLOOR, ffloor>;

llvm/lib/Target/AArch64/AArch64InstrGISel.td

 def : GINodeEquiv<G_DUPLANE32, AArch64duplane32>;
 def : GINodeEquiv<G_DUPLANE64, AArch64duplane64>;
 def : GINodeEquiv<G_TRN1, AArch64trn1>;
 def : GINodeEquiv<G_TRN2, AArch64trn2>;
 def : GINodeEquiv<G_EXT, AArch64ext>;
 def : GINodeEquiv<G_VASHR, AArch64vashr>;
 def : GINodeEquiv<G_VLSHR, AArch64vlshr>;
 
+def : GINodeEquiv<G_EXTRACT_VECTOR_ELT, vector_extract>;
+
 // These are patterns that we only use for GlobalISel via the importer.
 def : Pat<(f32 (fadd (vector_extract (v2f32 FPR64:$Rn), (i64 0)),
                      (vector_extract (v2f32 FPR64:$Rn), (i64 1)))),
            (f32 (FADDPv2i32p (v2f32 FPR64:$Rn)))>;

llvm/utils/TableGen/GlobalISelEmitter.cpp

Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderers(
   const CodeGenInstruction *DstI = DstMIBuilder.getCGI();
   CodeGenInstruction *OrigDstI = &Target.getInstruction(Dst->getOperator());
 
   StringRef Name = OrigDstI->TheDef->getName();
   unsigned ExpectedDstINumUses = Dst->getNumChildren();
 
   // EXTRACT_SUBREG needs to use a subregister COPY.
   if (Name == "EXTRACT_SUBREG") {
+    if (!Dst->getChild(1)->isLeaf())
+      return failedImport("EXTRACT_SUBREG child #1 is not a leaf");
     DefInit *SubRegInit = dyn_cast<DefInit>(Dst->getChild(1)->getLeafValue());
     if (!SubRegInit)
       return failedImport("EXTRACT_SUBREG child #1 is not a subreg index");
 
     CodeGenSubRegIndex *SubIdx = CGRegs.getSubRegIdx(SubRegInit->getDef());
     TreePatternNode *ValChild = Dst->getChild(0);
     if (!ValChild->isLeaf()) {
       // We really have to handle the source instruction, and then insert a