Page MenuHomePhabricator

[CodeGen] In narrowExtractedVectorLoad bail out for scalable vectors
ClosedPublic

Authored by david-arm on Jul 16 2020, 7:05 AM.

Details

Summary

In narrowExtractedVectorLoad there is an optimisation that tries to
combine extract_subvector with a narrowing vector load. At the moment
this produces warnings due to the incorrect calls to
getVectorNumElements() for scalable vector types. I've got this
working for scalable vectors too when the extract subvector index
is a multiple of the minimum number of elements. I have added a
new variant of the function:

MachineFunction::getMachineMemOperand

that copies an existing MachineMemOperand, but replaces the pointer
info with a null version since we cannot currently represent scaled
offsets.

I've added a new test for this particular case in:

CodeGen/AArch64/sve-extract-subvector.ll

Diff Detail

Event Timeline

david-arm created this revision.Jul 16 2020, 7:05 AM
david-arm updated this revision to Diff 282527.Aug 3 2020, 1:46 AM

LGTM, thanks!

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19341

Can you make this ElementCount, to make it clear that it can be scalable?

david-arm updated this revision to Diff 283261.Aug 5 2020, 8:47 AM
david-arm added a reviewer: efriedma.
david-arm added inline comments.Aug 6 2020, 11:01 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19341

Hi Sander, I thought about this and I'm not sure if this is quite the right approach either at the moment. I totally agree that it's good to make more use ElementCount objects directly, but I'd also like to avoid referring to ElementCount.Min everywhere as I personally don't think it's sensible to have public member variables in ElementCount. One reason for using getVectorMinNumElements() here is to avoid that ugliness, at least for now. What do you think? If you still want me to use EC.Min here instead of NumElts then that's fine, and regardless I think it's worth creating a follow-on patch that makes ElementCount members private.

This revision is now accepted and ready to land.Aug 12 2020, 1:55 AM