Page MenuHomePhabricator

[SVE][CodeGen] Fix TypeSize/ElementCount related warnings in sve-split-store.ll
Needs ReviewPublic

Authored by david-arm on Tue, Sep 1, 4:05 AM.

Details

Summary

I have fixed up a number of warnings resulting from TypeSize -> uint64_t
casts and calling getVectorNumElements() on scalable vector types. I
think most of the changes are fairly trivial except for those in
DAGTypeLegalizer::SplitVecRes_MSTORE I've tried to ensure we create
the MachineMemoryOperands in a sensible way for scalable vectors.

I have added a CHECK line to the following test:

CodeGen/AArch64/sve-split-store.ll

that ensures no new warnings are added.

Diff Detail

Event Timeline

david-arm created this revision.Tue, Sep 1, 4:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
david-arm requested review of this revision.Tue, Sep 1, 4:05 AM

I see no CHECK line for warnings in sve-split-store.ll?

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
545

nit: s/StoreSize/StSize for consistency?

david-arm updated this revision to Diff 289909.Fri, Sep 4, 3:44 AM
  • Renamed StoreSize -> StSize.
  • Added check for no warnings in sve-split-store.ll
david-arm marked an inline comment as done.Fri, Sep 4, 3:44 AM
efriedma added inline comments.Fri, Sep 11, 4:39 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
545

I don't think it's possible for StVT to be a vector type here.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2235

I don't think we should ever get here with a scalable vector? We should guard the creation of DemandedElts.

david-arm added inline comments.Mon, Sep 14, 12:35 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
545

I think the final else case covers vectors, right? We can have truncating vector stores. With the way the code is structured we test for the scalar cases first and therefore we were hitting the TypeSize->uint64_t cast warnings before we got to the final else case. That's why I converted the code to using TypeSize here. Alternatively, if you want I can restructure the code along the following lines:

if (StVT.isVector) {

...
return ...;

}

unsigned StWidth = StVT.getSizeInBits().getFixedSize();
unsigned StSize = StVT.getStoreSizeInBits().getFixedSize();
... deal with scalar cases ...

sdesmalen added inline comments.Mon, Sep 14, 6:24 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
558

You're using getKnownMinSize() here but using getFixedSize() below on line 560. Is this missing a condition, e.g.:

!StVT.isScalableVector() && !isPowerOf2_64(StWidth.getFixedSize())

or perhaps (because it seems like this only applies to integers):

!StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())
david-arm marked an inline comment as done.Mon, Sep 14, 6:31 AM
david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
558

Yes, I used the known min size here because it could be a scalable vector (since we haven't hit the final else case yet). The equivalent load function has the same problem (see https://reviews.llvm.org/D86697 that was checked in recently). So we either:

  1. Restructure the code so that the vector case comes first.
  2. Use getKnownMinSize.
  3. Use !StVT.isVector() && !isPowerOf2_64(StWidth.getFixedSize())

I'm happy to go with either way - whichever you and @efriedma prefer really.

david-arm updated this revision to Diff 291559.Mon, Sep 14, 6:37 AM
david-arm marked an inline comment as done.
  • In 3-op version of GetDemandedBits replaced the isScalableVector check with a check for non-zero DemandedElts.
david-arm marked an inline comment as done.Mon, Sep 14, 6:37 AM
sdesmalen added inline comments.Mon, Sep 14, 7:29 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
558

3 would have my preference.

efriedma added inline comments.Mon, Sep 14, 12:06 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
545

We can't have illegal truncating vector stores here; they have to be handled by LegalizeVectorOps.

I guess because of the way the code is structured, we don't check whether the store is legal until later, so we can have a legal truncating vector store here? Which is weird, but I guess you don't need to restructure the code in this patch.

558

3 is fine. Ideally, we'd refactor the code, I guess, but better to keep that separate.

(Theoretically, it's also possible to have a legal truncstore with a three-element vector, although no in-tree target does that. Since we're messing with the logic anyway, might as well handle that correctly.)

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2457–2458

Do we need to fix Alignment here?

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2235

Is if (!DemandedElts) related to this patch somehow?

david-arm added inline comments.Tue, Sep 15, 12:33 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2235

Hi @efriedma, your first comment here said "We should guard the creation of DemandedElts" so I thought this is what you meant. Are you saying that I shouldn't have to change anything at all in this function and that all callers of this function should create an empty DemandedElts for scalable vectors?

efriedma added inline comments.Wed, Sep 16, 7:33 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2235

You shouldn't have to change anything at all; the callers should deal with it.

david-arm updated this revision to Diff 292730.Fri, Sep 18, 2:44 AM
  • Removed the changes to 3-op version of GetDemandedBits
david-arm marked 3 inline comments as done.Fri, Sep 18, 3:08 AM
david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2457–2458

I'm not sure to be honest. We didn't fix this up in other places (e.g. https://reviews.llvm.org/D86697), such as SpltVecOp_STORE, SplitVecRes_MLOAD. I'm happy to do something with this though if you think it's needed?

efriedma added inline comments.Fri, Sep 18, 12:48 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2457–2458

I missed it on D86697; we do need the update the alignment.

There's a relationship between the MachinePointerInfo and the specified alignment: the alignment is actually the alignment of the base pointer of the MachinePointerInfo, not the alignment of the operation itself. So anywhere that we're throwing away the MachinePointerInfo, we also need to adjust the alignment to reflect the actual alignment of the operation.

david-arm updated this revision to Diff 293162.Mon, Sep 21, 7:19 AM
  • Updated alignment for HiMem store when dealing with scalable vectors.
david-arm marked an inline comment as done.Mon, Sep 21, 7:20 AM

Hi @efriedma I've tried to update the alignment for the HiMem scalable vector store. Is this new code what you were expecting here? I guess we'll need a new patch to clean up all the other split load/store cases too.