Page MenuHomePhabricator

[SVE] Disable INSERT_SUBVECTOR DAGCombine for scalable vectors
ClosedPublic

Authored by cameron.mcinally on Aug 31 2020, 1:29 PM.

Details

Summary

Looking for some input on this combine wrt scalable vectors. Right now, it uses code like this:

      unsigned NumElts = VT.getVectorNumElements();
...
        if ((NumElts % Scale) == 0 && (InsIdx % Scale) == 0) {

So it's obviously not compatible with scalable vectors right now.

Has anyone thought through overloading % for ElementCount? At first glance, it seems like we can't since we don't know the vscale. Am I missing something there?

Diff Detail

Event Timeline

cameron.mcinally requested review of this revision.Aug 31 2020, 1:29 PM

Given the way INSERT_SUBVECTOR is defined, I think this transform is essentially legal if you just make NumElts an ElementCount.

ElementCount already has operator/(unsigned RHS); it would probably make sense to add a corresponding ElementCount::divisibleBy(unsigned) or something like that.

Ok, I guess that makes sense. So if we're considering Min elements, should we just overload % then?

Not sure we want an actual operator%. If Min is divisible by RHS, we can divide without knowing the vscale. If Min isn't divisible by RHS, though, operator/ and operator% don't make sense.

I don't really have an opinion on any of this. Just thinking aloud...

If we did go with operator%, I'm not sure it's much more wrong than operator/ for scalable vector types. Assuming that the users are checking for EC % X == 0, we should be conservatively correct checking MIN % X == 0.

It would be nice if we didn't need to rewrite all the % uses to ElementCount::divisibleBy(unsigned) in existing code.

Hi, there was quite a lot of discussion about the use of operators in ElementCount recently (see https://reviews.llvm.org/D86065). I think the consensus was that this needs quite a bit more thought and discussion. My patch was accepted on the basis that we'd revisit this and come up with a better plan going forward. The general problem with "/" and "%" operators is that the caller needs to be careful if the operation is lossy. I introduced the "/" operator for a specific case of dividing by 2, where the Min value was known in advance to be a power of 2. I was thinking to raise this topic at the next SVE sync call. Looking at the code above it seems like all we're asking is if one thing is a multiple of another, which could be done with a function such as isKnownMultipleOf(...), where returning true guarantees we know at compile time the answer to be true.

Thanks, David. Ok, I'll put a pin in this until Thursday's call. But yeah, isKnownMultipleOf(...) sounds fair. I'll prepare a patch to see if any surprises come up.

Updated with @david-arm's comments for Thursday's Sync-up call.

Is there any way to write a test for the transform? Maybe not, given the limited places we use INSERT_SUBVECTOR.

The new API looks good.

Is there any way to write a test for the transform? Maybe not, given the limited places we use INSERT_SUBVECTOR.

There are a good number of existing tests that exercise this transform.

Also, D85364 is currently blocked by this Diff, so there are some new tests coming shortly.

efriedma accepted this revision.Sep 1 2020, 1:17 PM

LGTM

This revision is now accepted and ready to land.Sep 1 2020, 1:17 PM