This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] combine trunc(trunc) pattern
ClosedPublic

Authored by gargaroff on Mar 23 2020, 5:03 AM.

Details

Summary

Legalization can introduce the trunc(trunc) pattern. This can cause
problems if one of these intermediate truncs is not legal.
Combine truncs of this pattern, if the resulting trunc is legal.

Diff Detail

Event Timeline

gargaroff created this revision.Mar 23 2020, 5:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2020, 5:03 AM
arsenm added inline comments.Mar 23 2020, 10:23 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
286–287

Scalar truncate always needs to be legal?

gargaroff added inline comments.Mar 23 2020, 10:36 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
286–287

Maybe I misunderstood the official docs. There it says for the minimal rule set:

G_TRUNC must be legal for all inputs from the producer type set and all smaller outputs from the consumer type set.

Is this out-of-date or misleading? If so, I'll remove the check (or maybe assert it). Downstream we were actually planning on making our G_TRUNC only legal if it fits our register size (with s1 being an exception), which is why we need this combine rule. If G_TRUNC always needs to be legal, should this be reflected in the documentation?

arsenm added inline comments.Mar 30 2020, 7:32 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
286–287

I'm not sure. That rule doesn't reflect what is implemented in the in-tree targets. AArch64 for example does getActionDefinitionsBuilder(G_TRUNC).alwaysLegal(). For AMDGPU, I'm decomposing any vector truncates, but allowing any scalar up to the maximum bitwidth. One case I've been taking care around is degenerate, impossible to legalize cases. If you have an implicit use on a target instruction with an illegal truncate result type, we should probably just accept it regardless of legality (and not infinite loop in the legalizer).

gargaroff updated this revision to Diff 253799.Mar 31 2020, 1:12 AM
gargaroff marked an inline comment as done.

remove legality check for resulting combined trunc

gargaroff marked 3 inline comments as done.Mar 31 2020, 1:12 AM
gargaroff added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
286–287

I see, that is a good point. I guess we could safely remove this check. Most (all?) of the time the final remaining truncates after legalization should all result in legal result types anyway (thanks to the artifact combiner). If you somehow end up in a situation where all your instructions are already legal but a truncate is not, then you violated the rule that truncate must be legal for all smaller outputs from the consumer type set.

I guess that because truncates where not combined so far, you would often end up with this trunc(trunc) pattern which more or less forces you to have an alwaysLegal rule, because now suddenly the producer-set of trunc becomes the consumer-set of it too.

If you have an implicit use on a target instruction with an illegal truncate result type, we should probably just accept it regardless of legality

Wouldn't you say that an implicit-use is still a consumer and should therefore be legal according to the rule in the docs? Doesn't really matter here anymore, since I will remove the check anyway, but would be interesting to hear your opinion on this.

gargaroff marked an inline comment as done.Apr 7 2020, 12:42 AM

@arsenm Is this good to go then or is there something else?

arsenm accepted this revision.Apr 7 2020, 8:17 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
286–287

Wouldn't you say that an implicit-use is still a consumer and should therefore be legal according to the rule in the docs? Doesn't really matter here anymore, since I will remove the check anyway, but would be interesting to hear your opinion on this.

Yes, although it's a bit contrived since any possible smaller type needs to be handled. Every target has unlegalizable consumers through arbitrary implicit uses

This revision is now accepted and ready to land.Apr 7 2020, 8:17 AM
This revision was automatically updated to reflect the committed changes.