[PATCH] D76601: [GlobalISel] combine trunc(trunc) pattern

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:258-259
+      const LLT TruncSrcTy = MRI.getType(TruncSrc);
+      if (!isInstLegal({TargetOpcode::G_TRUNC, {DstTy, TruncSrcTy}}))
+        return false;
+
----------------
gargaroff wrote:
> arsenm wrote:
> > gargaroff wrote:
> > > arsenm wrote:
> > > > Scalar truncate always needs to be legal?
> > > 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?
> > 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).
> 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.
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76601