Page MenuHomePhabricator

[CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost
ClosedPublic

Authored by CarolineConcatto on Nov 25 2020, 5:30 AM.

Details

Summary

This patch replaces FixedVectorType by VectorType in getIntrinsicInstrCost
in BasicTTIImpl.h. It re-arranges the scalable type test earlier return
and add tests for scalable types.
This patch is the first patch of many to fix getIntrinsicInstrCost for scalable
vector.
Patches to fix the intrinsics masked gather and masked scatter, vector reduce
and fshl/fshr will be in upcoming patches.

Depends on D91532

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2020, 5:30 AM
CarolineConcatto requested review of this revision.Nov 25 2020, 5:30 AM

I think it would be a good idea to test that the paths through getCmpSelInstrCost and getArithmeticInstrCost work too.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1341

This can be hoisted above the loop, which is makes it a bit more clear that we're not handled scalarization here.

1345

nit: no need for brackets and shouldn't this just be kept as FixedVectorType anyway?

Thank you @samparker for your review. I agree with you that we should also test the code path of getCmpSelInstrCost and getArithmeticInstrCost, but at the moment these code paths were/are broken for scalable vector.
We are working to make these paths work for scalable vectors by replacing FixedVector with VectorType and using the ElementType class.
These will be addressed in upcoming patches. I don’t think that we should fix these code paths in this patch because the chain to fix it is long and will make the patch long and complex
What I can do is add tests with XFAIL for now, and remove in the future.
Is this a good solution for now?

I'd prefer if we didn't knowingly create broken paths here... does it currently just crash in the AArch64 backend or are there also failures in the generic parts? I'm thinking that we could just have a separate switch statement which checks for supported opcodes for scalable vectors and protects us from going down bad paths. A TODO comment could also be added to document why and what needs fixing up.

I'd prefer if we didn't knowingly create broken paths here

It is unfortunate, but this is all fallout from the VectorType refactor. All of this code was calling getNumElements() without checking isScalable(). The general approach for doing the VectorType refactor was "if it unconditionally calls getNumElements, make it unconditionally cast to FixedVectorType. If it was broken before, it will still be broken now, but at least we'll get an assertion failure instead of it just silently miscompiling"

Unfortunately, that means that a great deal of this TTI stuff is broken for scalable vectors, since it just wasn't being exercised.

Once D91174 is merged, I think it would be useful if somebody went through and updated the default TTI stuff to just return an invalid cost before any unconditional cast to FixedVectorType. At this point, we can get the stub of the comprehensive scalable vector TTI test, that initially just checks that most things return an invalid cost. (A few things do currently work)

  • fix nit
  • remove failing tests
CarolineConcatto edited the summary of this revision. (Show Details)Dec 3 2020, 2:16 AM
CarolineConcatto edited the summary of this revision. (Show Details)

I'd prefer if we didn't knowingly create broken paths here

It is unfortunate, but this is all fallout from the VectorType refactor. All of this code was calling getNumElements() without checking isScalable(). The general approach for doing the VectorType refactor was "if it unconditionally calls getNumElements, make it unconditionally cast to FixedVectorType. If it was broken before, it will still be broken now, but at least we'll get an assertion failure instead of it just silently miscompiling"

Unfortunately, that means that a great deal of this TTI stuff is broken for scalable vectors, since it just wasn't being exercised.

That's right, the paths are already broken. This patch merely tries to fix up a few code-paths that specifically don't require FixedVectorType. In this case, only the scalarization strictly requires FixedVectorType in this function. Other code-paths fail due to the called functions not yet working for scalable vectors, but those should be fixed in separate patches (e.g. one patch to address reductions in getTypeBasedIntrinsicInstrCost, one for gathers/scatters in getGatherScatterOpCost and one that fixes getArithmeticInstrCost for the codepath exercised by fshl/fshr).

Once D91174 is merged, I think it would be useful if somebody went through and updated the default TTI stuff to just return an invalid cost before any unconditional cast to FixedVectorType. At this point, we can get the stub of the comprehensive scalable vector TTI test, that initially just checks that most things return an invalid cost. (A few things do currently work)

Yes we should do that. From having seen how this work is layered, it will be quite a substantial task, as it requires changing the interfaces of the TTI functions and all its callers. We already started to change some of the 'root' nodes of the callgraph in e.g. D92178 (LoopVectorize), D92238 (SCEVExpander) and the SLPVectorizer, as those changes are NFC until any of the TTI functions returns an Invalid, and we can already have it do the sensible thing (i.e. fallback to "not vectorize"). The TTI methods themselves should either propagate the Invalid state (which hopefully comes for free), or as you suggested, return an Invalid cost. When the TTI functions start returning Invalid, we'll need to add additional tests for the Vectorizer and SLPVectorizer to guard they're indeed doing the right thing.

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1341

+1

1345

+1

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll
15

even though the cost doesn't directly matter much, it may be useful to just fill in the value it uses currently, so that when we improve the cost-model for scalable vectors, we can see the changes we make.

sdesmalen added inline comments.Dec 3 2020, 2:30 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1341

This should still use FixedVectorType

-address review's comment

llvm/include/llvm/CodeGen/BasicTTIImpl.h
1341

Thank you @sdesmalen. I've missed that.

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll
15

Fair enough! I've added the values.

... Yes we should do that. From having seen how this work is layered, it will be quite a substantial task ...

Yeah, the TTI changes were one of the most painful parts of the VectorType refactor. From my experience, you're not going to be able to break that up into small patches. There's a billion headers, and they're all tightly coupled. But at some point somebody has to go and update them to return InstructionCost anyways, that seems like a good time to do it.

I am not looking forward to resolving that merge conflict in my downstream...

I see a bunch of new intrinsics being evaluated, but only tests for CTLZ and CCTZ. Do the others have coverage somewhere already?

Hi @ctetreau.
I don't know if you are asking tests for Scalable Vector or FixedVectorType.
For Scalable Vector:
I added on the commit message which intrinsics will follow this patch because these intrinsics are broken for scalable vector.
So new tests with scalable vector will fail.
The only intrinsic that works now for scalable vector is CCTT and CTLZ.
For FixedVectorType:
I believe that only gather and scatter does not have tests for FixedVector, but all the others have tests.

Hi @ctetreau.
I don't know if you are asking tests for Scalable Vector or FixedVectorType.

As far as I can tell, this patch is only a behavioral change for scalable vectors, so those are the only tests the I'm asking about.

For Scalable Vector:
I added on the commit message which intrinsics will follow this patch because these intrinsics are broken for scalable vector.
So new tests with scalable vector will fail.
The only intrinsic that works now for scalable vector is CCTT and CTLZ.

Prior to your change, this code bails before trying to evaluate the cost of any of the intrinsics in the switch. Now, the function will try to evaluate all of the intrinsics in the switch, but you're saying that of those, only CCTT and CTLZ actually work, and are being tested. I see this as a regression.

Please move the if (isa<ScalableVectorType>(RetTy)) return BaseT::getIntrinsicInstrCost(ICA, CostKind); bailing out logic into the beginning of each switch branch that does not work/is not currently tested. These can be removed on a case-by-case basis in the patches that fix these branches.

Please move the if (isa<ScalableVectorType>(RetTy)) return BaseT::getIntrinsicInstrCost(ICA, CostKind); bailing out logic into the beginning of each switch branch that does not work/is not currently tested. These can be removed on a case-by-case basis in the patches that fix these branches.

+1

bsmith added a subscriber: bsmith.Dec 9 2020, 2:46 AM
  • add if statement for scalable vector for Intrinsics that not work with vscale
sdesmalen added inline comments.Dec 10 2020, 8:19 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
1246

Can you rewrite this as:

if (isa<ScalableVectorType>(RetTy))
  return BaseT::getIntrinsicInstrCost(ICA, CostKind)

so that you don't need to indent all the other lines?

  • replace if statement by early return when facing scalable vector
sdesmalen added inline comments.Dec 10 2020, 9:22 AM
llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll
15

Sorry, one more comment, can you also check for the instruction that you're checking the cost for?

  • add llvm-ir instruction to the value in the cost model tests
CarolineConcatto marked an inline comment as done.Dec 11 2020, 8:00 AM
CarolineConcatto marked 5 inline comments as done.
CarolineConcatto marked an inline comment as done.Dec 14 2020, 5:57 AM
david-arm accepted this revision.Dec 14 2020, 6:20 AM

LGTM with the nits addressed!

llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll
16

nit: Can you fix the minor typo here - CHECK-NETX?

26

nit: Can you fix the minor typo here - CHECK-NETX?

This revision is now accepted and ready to land.Dec 14 2020, 6:20 AM

-fix the work NEXT in the test

CarolineConcatto marked 2 inline comments as done.Dec 14 2020, 7:39 AM
This revision was landed with ongoing or failed builds.Dec 16 2020, 5:08 AM
This revision was automatically updated to reflect the committed changes.