[PATCH] D93416: [GlobalISel] Map extractelt to G_EXTRACT_VECTOR_ELT

Mon Jan 11 16:26:13 PST 2021

bjope added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrGISel.td:167-168

+def : GINodeEquiv<G_EXTRACT_VECTOR_ELT, vector_extract>;
+
 // These are patterns that we only use for GlobalISel via the importer.
----------------
bjope wrote:
> dsanders wrote:
> > 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.
> 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.
> 
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93416/new/

https://reviews.llvm.org/D93416