Page MenuHomePhabricator

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

Authored by david-arm on Sep 1 2020, 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.Sep 1 2020, 4:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
david-arm requested review of this revision.Sep 1 2020, 4:05 AM

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

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

nit: s/StoreSize/StSize for consistency?

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

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.Sep 14 2020, 12:35 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
546

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.Sep 14 2020, 6:24 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
559

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.Sep 14 2020, 6:31 AM
david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
559

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.Sep 14 2020, 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.Sep 14 2020, 6:37 AM
sdesmalen added inline comments.Sep 14 2020, 7:29 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
559

3 would have my preference.

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

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.

559

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.Sep 15 2020, 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.Sep 16 2020, 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.Sep 18 2020, 2:44 AM
  • Removed the changes to 3-op version of GetDemandedBits
david-arm marked 3 inline comments as done.Sep 18 2020, 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.Sep 18 2020, 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.Sep 21 2020, 7:19 AM
  • Updated alignment for HiMem store when dealing with scalable vectors.
david-arm marked an inline comment as done.Sep 21 2020, 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.

sdesmalen added inline comments.Wed, Sep 30, 3:50 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
553

What guarantees that getFixedSize can be used here?

david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
553

I think @eli.friedman mentioned that in an earlier comment that was only for scalar cases, although I agree there are no checks to actually prevent this. I just assumed that we wouldn't be creating arbitrarily large integer types, i.e. 128, 256, etc. I can try adding an extra "!StVT.isVector()" check and see if any tests fail?

efriedma added inline comments.Wed, Sep 30, 3:09 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
553

It's implicitly prevented by the way legalization works. In LegalizaeDAG, we assume all vector operations are legal: any illegal vector ops should have been handled by LegalizeVectorOps.

(I don't want to get into a deep discussion of why it works this way, but essentially, the reason it works this way is to allow LegalizeVectorOps to generate operations that involve illegal types.)

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2449

The ABI alignment isn't relevant here. The relevant numbers are the specified alignment, and the size of LoMemVT. Basically, we need to reduce the alignment if it's bigger than the size of LoMemVT.

I'm sure we have at least one example of this elsewhere.

david-arm updated this revision to Diff 295549.Thu, Oct 1, 6:30 AM
david-arm marked an inline comment as done.
david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2449

OK, I'm struggling to know how to proceed here. I've tried again using a function called commonAlignment that works out the minimum of the original alignment and the LoMemVT size. Is this what you mean?

efriedma accepted this revision.Sun, Oct 4, 10:11 PM

LGTM

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2449

This looks right.

This revision is now accepted and ready to land.Sun, Oct 4, 10:11 PM