Page MenuHomePhabricator

[SVE] Fix invalid Scalable to fixed width vetor type demotion in LLT
AbandonedPublic

Authored by ctetreau on Jun 22 2020, 1:46 PM.

Details

Summary

getLLTForType calls LLT::vector(Ty->getNumElements(), ScalarTy) for
fixed width and scalable vectors, losing information on the scalability
of the input vector. Rather than miscompile, return an invalid LLT for
scalable vectors.

Diff Detail

Event Timeline

ctetreau created this revision.Jun 22 2020, 1:46 PM
Herald added a project: Restricted Project. · View Herald Transcript

This code still won't actually work, either way. (I think there's a patch out for review to disable GlobalISel for functions that contain scalable vectors?)

If you don't want to deal with it now, just do auto NumElements = cast<FixedVectorType>(VTy)->getNumElements();

This code still won't actually work, either way. (I think there's a patch out for review to disable GlobalISel for functions that contain scalable vectors?)

If you don't want to deal with it now, just do auto NumElements = cast<FixedVectorType>(VTy)->getNumElements();

Unforunately, that causes llvm\test\CodeGen\AArch64\sve-intrinsics-ld1.ll to fail. So it's either this or calling getElementCount().Min. (or adding the feature)

I guess getElementCount().Min would eliminate the behavior change, but it's clearly broken and I'm not sure it's worth preserving the original behavior.

I talked to Christopher offline, and he said this code is in fact getting hit with SVE enabled. Is D81557 working correctly?

I talked to Christopher offline, and he said this code is in fact getting hit with SVE enabled. Is D81557 working correctly?

We'd need to see a backtrace and repro.

Hi @aemerson @efriedma @ctetreau

#7 0x0000ffffa0f26760 llvm::getLLTForType(llvm::Type&, llvm::DataLayout const&) (/lib64/libc.so.6+0x36760)
#8 0x0000000002556f74 llvm::computeValueLLTs(llvm::DataLayout const&, llvm::Type&, llvm::SmallVectorImpl<llvm::LLT>&, llvm::SmallVectorImpl<unsigned long>*, unsigned long) (.localalias) (./bin/llc+0x2556f74)
#9 0x0000000001aeb108 llvm::IRTranslator::getOrCreateVRegs(llvm::Value const&) (.part.0) (./bin/llc+0x1aeb108)
#10 0x00000000019df8a4 llvm::IRTranslator::runOnMachineFunction(llvm::MachineFunction&) (./bin/llc+0x19df8a4)

So clearly we're not falling back on DAG ISel early enough. I'll try to address this in a separate patch.

This problem occurs in the IRTranslator just before we call lowerFormalArguments (which is currently the earliest point we detect scalable types and fall back to DAG ISel)

Perhaps this change is good enough? We call computeValueLLTs just before lowerFormalArguments, which will detect the scalable type and fall back anyway even if the LLT type is wrong. The other option is to add yet another fallBackOnDAGISel type callback that allows the backend to reject certain types before we create the vregs.

Perhaps this change is good enough? We call computeValueLLTs just before lowerFormalArguments, which will detect the scalable type and fall back anyway even if the LLT type is wrong. The other option is to add yet another fallBackOnDAGISel type callback that allows the backend to reject certain types before we create the vregs.

I think we just need to move the check from lowerFormalArguments to the for-loop before the call to it, and emit a remark to explain why we're falling back.

Hi @aemerson @ctetreau, I have a fix here that prevents us entering this code for scalable types:

https://reviews.llvm.org/D82524

This can be abandoned now?

ctetreau abandoned this revision.Thu, Jul 9, 12:42 PM

This patch (in the form of an unconditional cast to FixedVectorType on line 22) has been rolled into D82210