This is an archive of the discontinued LLVM Phabricator instance.

[DemandedBits][BDCE] Support vectors of integers
ClosedPublic

Authored by nikic on Dec 4 2018, 2:16 PM.

Details

Summary

This came up during D54876. DemandedBits and BDCE currently only support scalar integers. This patch extends them to also handle vector integer operations. In this case the demanded bits are tracked for all lanes at once (as is done for known bits and demanded bits analyses in other places).

The implementation is mostly a matter of going from isIntegerTy to isIntOrIntVectorTy and switching some ConstantInt dyn_casts to use match instead.

I think the main notable change here from an API perspective is that the getDemandedBits() method now a) requires the instruction to have an int/vec-of-int type and b) returns a "scalar size in bits" value. Previously it allowed any sized instruction and returned a "type size in bits" value. In particular, for vectors this method would have previously returned a num elements * scalar size large value (of 1s). I checked existing uses of getDemandedBits() and from what I can determine none will be invoked on anything other than integer instructions. Only BDCE passed non-integer instructions to it sometimes, but that's also adjusted as part of this patch.

Diff Detail

Event Timeline

nikic created this revision.Dec 4 2018, 2:16 PM
hfinkel added inline comments.Dec 5 2018, 4:08 PM
test/Analysis/DemandedBits/vectors.ll
8

I'm missing something here. Shouldn't there be two groups of demanded bits here, one group per vector lane, so that the overall demanded bits looks something like: 0x000000ff000000ff?

nikic marked an inline comment as done.Dec 6 2018, 2:52 AM
nikic added inline comments.
test/Analysis/DemandedBits/vectors.ll
8

The demanded bits are tracked for all vector lanes together. A bit is demanded if it is demanded in any of the vector lanes.

Other passes that do similar things (computeKnownBits in ValueTracking and SimplifyDemandedBits in InstCombine) take the same approach. I assume that because it's usually not worth the additional computational cost (and in this case also memory usage) to track bits for individual lanes. It also simplifies things for consumers of the analysis, as it allows treating scalar and vectors in the same way.

RKSimon added inline comments.Dec 6 2018, 4:26 AM
test/Analysis/DemandedBits/vectors.ll
8

I raised https://bugs.llvm.org/show_bug.cgi?id=36319 about at least adding the same DemandedElts argument support to ValueTracking that we have with the SelectionDAG computeKnownBits/ComputeNumSignBits equivalents.

I have toyed with the idea of adding a DemandedElts argument to SimplifyDemandedBits as well but haven't pursued it yet (so not completely "per bit" but at least limited to shared demandedbits from the vector elements we care about).

nikic marked an inline comment as done.Dec 6 2018, 10:09 AM
nikic added inline comments.
test/Analysis/DemandedBits/vectors.ll
8

That's interesting, I wasn't aware that SelectionDAG also tracks demanded elements. This is probably more valuable for cases where you look at a single use-site (what computeKnownBits does) than an analysis such as this one (which considers the whole function, so e.g. all extractelements will end up as demanded elements, with no distinction for which one demands what).

hfinkel accepted this revision.Dec 6 2018, 10:40 AM

This LGTM. However, please, in llvm/Analysis/DemandedBits.h, document what this means for vectors (that it treats all elements the same and returns a mask of the scalar size). You might expand on the comment:

/// Return the bits demanded from instruction I.
APInt getDemandedBits(Instruction *I);
This revision is now accepted and ready to land.Dec 6 2018, 10:40 AM
This revision was automatically updated to reflect the committed changes.
nikic reopened this revision.Dec 7 2018, 6:13 AM

Reverted this shortly afterwards via rL348558 due to assertion failures, so reopening this revision.

This revision is now accepted and ready to land.Dec 7 2018, 6:13 AM
nikic updated this revision to Diff 177197.Dec 7 2018, 6:21 AM

Remove the requirement that the instruction passed to getDemandedBits() must have integer or vector of integer type. While the analysis only deals with these instructions, it is more convenient for consuming code to not have type restrictions.

Add a LoopVectorize test case which triggered an assertion with the previous implementation, as it called getDemandedBits() on a pointer-typed instruction.

hfinkel accepted this revision.Dec 7 2018, 6:33 AM

LGTM

This revision was automatically updated to reflect the committed changes.