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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders 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 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93416