This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Make s8 and s16 G_CONSTANTs legal
ClosedPublic

Authored by aemerson on Jun 19 2019, 10:54 PM.

Details

Summary

We sometimes get poor code size because constants of types < 32b are legalized
as 32 bit G_CONSTANTs with a truncate to fit. This works but means that the
localizer can no longer sink them (although it's possible to extend it to do so).

On AArch64 however s8 and s16 constants can be selected in the same way as s32
constants, with a mov pseudo into a W register. If we make s8 and s16 constants
legal then we can avoid unnecessary truncates, they can be CSE'd, and the
localizer can sink them as normal.

There is a caveat: if the user of a smaller constant has to widen the sources,
we end up with an anyext of the smaller typed G_CONSTANT. This can cause
regressions because of the additional extend and missed pattern matching. To
remedy this, there's a new artifact combiner to generate the wider G_CONSTANT
if it's legal for the target.

Diff Detail

Event Timeline

aemerson created this revision.Jun 19 2019, 10:54 PM

@arsenm This change also adds a generic artifact combiner affecting all targets including AMDGPU.

dsanders added inline comments.Jun 20 2019, 10:22 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
66–81

Could you elaborate on why this should be in the artefact combiner rather than the post-legalizer combiner? It's targeting the anyext produced by legalization so I guess it makes sense to include it here but what kind of size-explosion do we get without it?

Also there's a catch with this. If there's multiple users require different sizes then the constant will be duplicated potentially costing more in duplicated materialization code than the extend would have.

Should something like D56706 go along with this?

arsenm added inline comments.Jun 20 2019, 10:38 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
73–76

I thought G_CONSTANT only allowed CImm sources.

You should be able to move the .sext() into the Cimm case instead of always applying it

aemerson marked an inline comment as done.Jun 20 2019, 10:50 AM

Should something like D56706 go along with this?

Maybe?

@dsanders Should we have a generic post legalizer combiner pass now?

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
66–81

Pretty much as you said, because the anyext originates from the legalizer. I wouldn't say there's a size explosion, but removing unnecessary extends is a good thing and allows passes like the localizer to deal with the constant easier.

As for multiple users, yes that's potentially possible but in practice it's more beneficial overall for code size to do this than not, especially with the imported patterns which don't look through these extends. There are only a few different type combinations that a constant can take after all.

Should something like D56706 go along with this?

Maybe?

@dsanders Should we have a generic post legalizer combiner pass now?

A generic post legalizer combiner is tricky because targets often want very different things. The plan is to have generic rules that target specific combiners can opt into in a reasonable way (e.g. by groups based on a generalized trait of the target) via the CombinerHelper

Should something like D56706 go along with this?

Maybe?

@dsanders Should we have a generic post legalizer combiner pass now?

A generic post legalizer combiner is tricky because targets often want very different things. The plan is to have generic rules that target specific combiners can opt into in a reasonable way (e.g. by groups based on a generalized trait of the target) via the CombinerHelper

Alright, we'll keep it in the artifact combiner then.

@paquette this ok from the arm64 side?

paquette accepted this revision.Jun 21 2019, 9:00 AM

I think that this is reasonable. LGTM

This revision is now accepted and ready to land.Jun 21 2019, 9:00 AM
This revision was automatically updated to reflect the committed changes.