Page MenuHomePhabricator

[CodeGen] Let computeKnownBits do something sensible for scalable vectors
ClosedPublic

Authored by david-arm on May 22 2020, 6:14 AM.

Details

Summary

Until we have a real need for computing known bits for scalable
vectors I have simply changed the code to bail out for now and
pretend we know nothing. I've also fixed up some simple callers of
computeKnownBits too.

Diff Detail

Event Timeline

david-arm created this revision.May 22 2020, 6:14 AM
david-arm retitled this revision from [CodeGen] Add support for extracting elements of scalable vectors to [CodeGen] Let computeKnownBits do something sensible for scalable vectors.May 22 2020, 6:15 AM
david-arm edited the summary of this revision. (Show Details)

I think we should be able to make computeKnownBits work to some extent, but I guess it's not high priority at the moment.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2278

If we're going to bail out anyway, can we bail out explicitly, instead of waiting for computeKnownBits to bail out?

david-arm marked an inline comment as done.Jun 1 2020, 5:57 AM
david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2278

I thought about that, but then I was also worried about duplicating the behaviour of the called routines in the callers. If the behaviour of MaskedValueIsZero or computeKnownBits changes, i.e. how we return values and so on, then this becomes out of sync. If you don't think that's a problem I can bail out early as you suggested?

Hi @efriedma, I don't suppose you've had any further thoughts about my reply to your comment? I was specifically worried that also bailing out early for all functions that call computeKnownBits means we're reliant on the behaviour of computeKnownBits not changing, i.e. in terms of it's returned values. If you don't think this is a concern, then I'm happy to do that.

It's not clear how we actually want to represent DemandedElts for scalable vectors. I think we'd go through all the callers before changing anything in that respect, so explicitly bailing out seems better than intentionally synthesizing something which might be wrong anyway.

For MaskedValueIsZero specifically, maybe makes sense to change the implementation to return Mask.isSubsetOf(computeKnownBits(V, Depth).Zero);, instead of trying to synthesize DemandedElts.

david-arm updated this revision to Diff 269426.Tue, Jun 9, 12:43 AM

Hi @efriedma, thanks for the reply! Your comment about changing MaskedValueIsZero makes a lot of sense and improves code reuse. Hopefully the patch looks better now!

The code looks right.

For both this and the SimplifyDemandedBits/VectorElts patches, I'm not sure what we want to do about testing. Currently, I assume some optimizations "work", and this breaks them. And probably we do some incorrect optimizations. It would be nice if we could have at least a little test coverage showing these changes have some effect.

david-arm updated this revision to Diff 269790.Wed, Jun 10, 4:44 AM

Hi @efriedma, I tried to find some pre-existing folds that we were doing before my patch, but couldn't find anything. The problem is that fundamentally many of these functions need to understand SPLAT_VECTORs in order to return something known. I tried adding SPLAT_VECTOR support to isConstOrConstSplat, since that allowed us to call MaskedValueIsZero a bit more. However, we then end up crashing in SimplifyDemandedBits. However, I do understand your concern so I have added a unit test that confirms we pretend we know nothing. This patch kills off the last remaining warnings in quite a few tests, so I have added checks for no warnings in those files.

This revision is now accepted and ready to land.Wed, Jun 10, 12:00 PM
This revision was automatically updated to reflect the committed changes.