This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeType] When LegalizeType procedure widens a masked_gather, set MemoryType's EltNum equal to Result's EltNum
ClosedPublic

Authored by yubing on Dec 20 2020, 10:30 PM.

Details

Summary

When LegalizeType procedure widens a masked_gather, set MemoryType's EltNum equal to Result's EltNum.

As I mentioned in https://reviews.llvm.org/D91092, in previous code, If we have a v17i32's masked_gather in avx512, we widen it to a v32i32's masked_gather with a v17i32's MemoryType. When the SplitVecRes_MGATHER process this v32i32's masked_gather, GetSplitDestVTs will assert fail since what you are going to split is v17i32.

Diff Detail

Event Timeline

yubing created this revision.Dec 20 2020, 10:30 PM
yubing requested review of this revision.Dec 20 2020, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2020, 10:30 PM
yubing retitled this revision from [LegalizeType] When it widens mgather in LegalizeType, set MemoryType equal to ResultType to [LegalizeType] When LegalizeType procedure widens a masked_gather, set MemoryType equal to ResultType.Dec 20 2020, 10:40 PM
yubing edited the summary of this revision. (Show Details)
yubing added reviewers: craig.topper, RKSimon.

Fix WidenVecOp_MSCATTER as well?

May should just take the element type from the original memory VT and the element count from result? Other targets have extending gathers I think which I think would have different element sizes between result and memory?

Fix WidenVecOp_MSCATTER as well?

May should just take the element type from the original memory VT and the element count from result? Other targets have extending gathers I think which I think would have different element sizes between result and memory?

Yeah, Besides If OpNo == 4 in WidenVecOp_MSCATTER, I guess we shouldn't widen MemType since we only widen the index?

Fix WidenVecOp_MSCATTER as well?

May should just take the element type from the original memory VT and the element count from result? Other targets have extending gathers I think which I think would have different element sizes between result and memory?

Yeah, Besides If OpNo == 4 in WidenVecOp_MSCATTER, I guess we shouldn't widen MemType since we only widen the index?

I think we should keep the same number of elements in the memory VT as the operands so I think it should be widened.

I'm starting to wonder if the real mistake was making memory VT a vector for gather/scatter. Maybe it should have just been a scalar for the element type?

yubing updated this revision to Diff 313056.Dec 21 2020, 2:53 AM

Modify code according to Craig's comments

Fix WidenVecOp_MSCATTER as well?

May should just take the element type from the original memory VT and the element count from result? Other targets have extending gathers I think which I think would have different element sizes between result and memory?

Yeah, Besides If OpNo == 4 in WidenVecOp_MSCATTER, I guess we shouldn't widen MemType since we only widen the index?

I think we should keep the same number of elements in the memory VT as the operands so I think it should be widened.

I'm starting to wonder if the real mistake was making memory VT a vector for gather/scatter. Maybe it should have just been a scalar for the element type?

Besides, do we use the information of MemType after LegalizeType procedure? I found X86InstrFragmentsSIMD.td may use it that info but mgather is handled outside X86DAGToDAGISel::SelectCode.

Fix WidenVecOp_MSCATTER as well?

May should just take the element type from the original memory VT and the element count from result? Other targets have extending gathers I think which I think would have different element sizes between result and memory?

Yeah, Besides If OpNo == 4 in WidenVecOp_MSCATTER, I guess we shouldn't widen MemType since we only widen the index?

I think we should keep the same number of elements in the memory VT as the operands so I think it should be widened.

I'm starting to wonder if the real mistake was making memory VT a vector for gather/scatter. Maybe it should have just been a scalar for the element type?

Besides, do we use the information of MemType after LegalizeType procedure? I found X86InstrFragmentsSIMD.td may use it that info but mgather is handled outside X86DAGToDAGISel::SelectCode.

X86 doesn't use it, but a target the support extending gather would need the memory VT to figure out how much it extends.

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

This needs to be widened for OpNo == 4 as well otherwise it would have a different element count.

Fix WidenVecOp_MSCATTER as well?

May should just take the element type from the original memory VT and the element count from result? Other targets have extending gathers I think which I think would have different element sizes between result and memory?

Yeah, Besides If OpNo == 4 in WidenVecOp_MSCATTER, I guess we shouldn't widen MemType since we only widen the index?

I think we should keep the same number of elements in the memory VT as the operands so I think it should be widened.

I'm starting to wonder if the real mistake was making memory VT a vector for gather/scatter. Maybe it should have just been a scalar for the element type?

Besides, do we use the information of MemType after LegalizeType procedure? I found X86InstrFragmentsSIMD.td may use it that info but mgather is handled outside X86DAGToDAGISel::SelectCode.

X86 doesn't use it, but a target the support extending gather would need the memory VT to figure out how much it extends.

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

Eh, You mean we should make MemType's EltNum equal to index's EltNum instead of ResultType's EltNum?
But in fact, index and result can have different element number and index's upper element is ignored as mentioned in https://reviews.llvm.org/D51337.

craig.topper added inline comments.Dec 21 2020, 6:25 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
4906

Oh you're right. I forgot that was allowed now. It used to not be in the original implementation. So disregard.

This revision is now accepted and ready to land.Dec 21 2020, 6:26 PM
yubing retitled this revision from [LegalizeType] When LegalizeType procedure widens a masked_gather, set MemoryType equal to ResultType to [LegalizeType] When LegalizeType procedure widens a masked_gather, set MemoryType's EltNum equal to Result's EltNum.Dec 21 2020, 6:32 PM
yubing edited the summary of this revision. (Show Details)

LGTM

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

Anyway, we solve the bug, cheers~

This revision was landed with ongoing or failed builds.Dec 21 2020, 9:28 PM
This revision was automatically updated to reflect the committed changes.