Page MenuHomePhabricator

[SelectionDAGLegalize] Improve promotion of CTLZ
AbandonedPublic

Authored by jonpa on Feb 13 2019, 2:07 PM.

Details

Summary

When CTLZ is first promoted from i16 to i32 by type-legalization, and then again from i32 to i64 during Legalization, two unfolded subtractions of constants will remain in output.

This was discussed on llvm-dev, see http://lists.llvm.org/pipermail/llvm-dev/2019-February/thread.html#130115. Sanjay pointed out that the reason for this missed optimization is that DAGCombiner::visitTRUNCATE() is restricted to handle this pre-legalize only.

As an alternative to enabling the DAGCombiner post-legalize also, this is a patch that addresses this issue directly by detecting this case during promotion of CTLZ and propagating the constant to the present add (of negative constant) instead of always creating a new sub.

Does this look reasonable? This is NFC on SystemZ/SPEC, but improves tests both on SystemZ and AMDGPU.

Diff Detail

Event Timeline

jonpa created this revision.Feb 13 2019, 2:07 PM

Adding more potential reviewers for legalization changes. I'm not sure how much combining/optimization we want to do in here.

Generally we should prefer to perform combines in DAGCombine in cases where it's straightforward. (There are a few scattered optimizations in legalization, but mostly for cases that can't simplify after legalization, like libcalls.)

@jonpa - IIRC, some targets infinite loop if we always allow the generic trunc fold in DAGCombiner after legalization, but you might be able to constrain it using the existing TLI hooks:
IsDesirableToPromoteOp()
isTypeDesirableForOp()

arsenm added inline comments.Feb 21 2019, 9:02 AM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4155

Can you do the sub in APInt and avoid getSExtValue?

jonpa abandoned this revision.Feb 25 2019, 8:58 AM

Generally we should prefer to perform combines in DAGCombine in cases where it's straightforward.

Given this, it seems reasonable to abandon this patch for https://reviews.llvm.org/D58521, which enables the DAGCombiner post-legalize which then handles this case.