Page MenuHomePhabricator

[GlobalISel] Map extractelt to G_EXTRACT_VECTOR_ELT
ClosedPublic

Authored by bjope on Dec 16 2020, 12:41 PM.

Details

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).

Diff Detail

Event Timeline

bjope created this revision.Dec 16 2020, 12:41 PM
bjope requested review of this revision.Dec 16 2020, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 12:41 PM
Herald added a subscriber: wdng. · View Herald Transcript
This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jan 11, 12:54 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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 added inline comments.Mon, Jan 11, 3:48 PM
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 added inline comments.Mon, Jan 11, 4:26 PM
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 added inline comments.Mon, Jan 11, 4:28 PM
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