This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Legalize integer across-lane intrinsics with actual type
ClosedPublic

Authored by dzhidzhoev on Aug 1 2023, 3:24 PM.

Details

Summary

Across-lane intrinsics with integer destination type (uaddv, saddv,
umaxv, smavx, uminv, sminv) were legalized with the destination type
given in the LLVM IR intrinsic’s definition. It was wider than
the actual destination type of the corresponding machine instruction.
InstructionSelect was implicitly supposed to generate underlying
extension instructions for these intrinsics, while the real destination
type was opaque for other GlobalISel passes. Thus,
llvm/test/CodeGen/AArch64/arm64-vaddv.ll failed on GlobalISel since
the generated code was worse in functions that used the value of
an across-lane intrinsic in following FP&SIMD instructions (functions
with _used_by_laneop suffix).

Here intrinsics are legalized and selected with an actual destination
type, making it transparent to other passes. If the destination
value is used in further instructions accepting FPR registers, there
won’t be extra copies across register banks. i16 type is added to
the list of the types of the FPR16 register bank to make it possible,
and a few SelectionDAG patterns are modified to eliminate ambiguity
in TableGen.

Diff Detail

Event Timeline

dzhidzhoev created this revision.Aug 1 2023, 3:24 PM
dzhidzhoev requested review of this revision.Aug 1 2023, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2023, 3:24 PM
arsenm added inline comments.Aug 1 2023, 3:27 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
328–341

This should be a separate patch from the AArch64 changes

dzhidzhoev updated this revision to Diff 546252.Aug 1 2023, 3:43 PM

Addressed @arsenm comment. Moved artifact combiner change to https://reviews.llvm.org/D156832.

dzhidzhoev edited the summary of this revision. (Show Details)Aug 1 2023, 3:44 PM
aemerson accepted this revision.Aug 15 2023, 1:34 PM

LGTM, thanks.

This revision is now accepted and ready to land.Aug 15 2023, 1:34 PM

Some of this looks a bit odd, with the use of fp registers in integer patterns. Can you change the references of bf16 to f16 in this patch? I think it makes a better default register class for f16 (but might not make any difference in practice), and some of the patterns are already acting upon f16 types.

Also, this might also be introducing illegal COPY instructions. The llvm-test-suite has been failing for a while, but after D157202 should be doing better. This looks like it might cause issues again though, if I am running it correctly?

Addressed @dmgreen comments.
Changed references of bf16 to f16.
Removed changes modifying RegBankSelect behaviour related to G_ZEXT. Used GIIgnoreCopies flag in patterns to select sext/zext(uaddv/saddv/...) used by
laneop.

This revision was landed with ongoing or failed builds.Aug 17 2023, 9:21 AM
This revision was automatically updated to reflect the committed changes.