This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Provide an universal interface for FixedVectorType::get. NFC.
ClosedPublic

Authored by HanKuanChen on Jun 29 2023, 10:22 AM.

Diff Detail

Event Timeline

HanKuanChen created this revision.Jun 29 2023, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 10:22 AM
HanKuanChen requested review of this revision.Jun 29 2023, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 10:22 AM
This revision is now accepted and ready to land.Jun 29 2023, 10:27 AM
aeubanks reopened this revision.Jun 29 2023, 9:49 PM
aeubanks added a subscriber: aeubanks.

this causes crashes on

$ /tmp/a.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define void @f() {
entry:
  %cmp.i.i.i = fcmp olt x86_fp80 0xK00000000000000000000, 0xK00000000000000000000
  %cmp.i6.i.i = fcmp olt x86_fp80 0xK00000000000000000000, 0xK00000000000000000000
  ret void
}
$ opt -p slp-vectorizer -disable-output /tmp/a.ll

I've reverted this for now

This revision is now accepted and ready to land.Jun 29 2023, 9:49 PM

(also I don't see how this is an improvement)

Update: Remove isValidElementType assert in makeScalarTyToVectorTy.

This revision was landed with ongoing or failed builds.Jun 29 2023, 11:16 PM
This revision was automatically updated to reflect the committed changes.

(also I don't see how this is an improvement)

The purpose is for revec.

nikic added a subscriber: nikic.Jun 30 2023, 2:40 AM

Reverted this again. Please make sure that reviews and commit messages always have a meaningful description.

This change doesn't make sense in isolation, and neither the patch nor the commit include the necessary larger context that could justify this change.

@aeubanks @nikic I will not add this in the commit message. It is too long.
Let's say we have a IR like this

%in1 = getelementptr inbounds i32, ptr %in0, i64 1
%in2 = getelementptr inbounds i32, ptr %in0, i64 2
%in3 = getelementptr inbounds i32, ptr %in0, i64 3
%v0 = load i32, ptr %in0, align 4
%v1 = load i32, ptr %in1, align 4
%v2 = load i32, ptr %in2, align 4
%v3 = load i32, ptr %in3, align 4

SLP can make this into load <4 x i32>, ptr %in0, align 4. Using FixedVectorType::get(i32, 4) works well.
For revec, we have a IR like this

%in1 = getelementptr inbounds i32, ptr %in0, i64 4
%in2 = getelementptr inbounds i32, ptr %in0, i64 8
%in3 = getelementptr inbounds i32, ptr %in0, i64 12
%v0 = load <4 x i32>, ptr %in0, align 4
%v1 = load <4 x i32>, ptr %in1, align 4
%v2 = load <4 x i32>, ptr %in2, align 4
%v3 = load <4 x i32>, ptr %in3, align 4

SLP can make this into load <16 x i32>, ptr %in0, align 4. But using FixedVectorType::get(<4 x i32>, 4) is incorrect. The actually function call should be FixedVectorType::get(i32, 16).
To make scalar to vector and vector to vector work together, you need

unsigned NumElements = 1;
if (auto *VecTy = dyn_cast<FixedVectorType>(ScalarTy))
  NumElements = VecTy->getNumElements();
FixedVectorType *VecTy = FixedVectorType::get(ScalarTy->getScalarType(), VF * NumElements);

If FixedVectorType::get occurs only few times, do in this way is better. But the fact is we need a function to wrap it.

nikic added a comment.Jun 30 2023, 5:37 AM

Here is how your patch description could have looked like:

In the future, we want to support ScalarTy being a vector, which is extended to a VectorTy with more elements. To allow making that change in a central place, introduce a makeScalarTyToVectorTy() helper.

Is that an accurate summary of the motivation?

Maybe together with a note:

This is part up an effort to upstream revec support.

If that is what you're doing.

vdmitrie added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
1082

my 2 cents

Based on recent comment this is going to be extended to handle vector type as well. In this regard name of the function is not going to sound accurate for its purpose.
I'd suggest to change its signature into something like
FixedVectorType *getWidenedType(const Type *Ty, unsigned VF)

and perhaps place it into
llvm/include/llvm/Analysis/VectorUtils.h
llvm/lib/Analysis/VectorUtils.cpp