[OpenCL] Disable vector to scalar types coercion for OpenCL #8160

cdai2 · 2023-01-31T08:35:45Z

For x86 target, vector types (both result and arguments) can be coerced to scalars of the same size, e.g:

  define zeroext i1 @_Z18convert_ulong4_rteDv4_t(<4 x i16> %x)
  ; becomes
  define zeroext i1 @_Z18convert_ulong4_rteDv4_t(i64 %x.coerced)

Such behavior is completely valid for x86, but the backend vectorizer cannot work with scalars instead of vectors.

With this patch, argument and result types will be leaved unchanged in the CodeGen.

New option fopencl-force-vector-abi is also added to force-disables vector to scalar coercion when provided.

For x86 target, vector types (both result and arguments) can be coerced to scalars of the same size, e.g: define zeroext i1 @_Z18convert_ulong4_rteDv4_t(<4 x i16> %x) ; becomes define zeroext i1 @_Z18convert_ulong4_rteDv4_t(i64 %x.coerced) Such behavior is completely valid for x86, but the backend vectorizer cannot work with scalars instead of vectors. With this patch, argument and result types will be leaved unchanged in the CodeGen. New option fopencl-force-vector-abi is also added to force-disables vector to scalar coercion when provided.

Fznamznon

That doesn't seem to be dependent on SYCL, why don't we commit it directly to LLORG instead?

cdai2 · 2023-01-31T10:23:23Z

That doesn't seem to be dependent on SYCL, why don't we commit it directly to LLORG instead?

We will land open source OpenCL CPU RT code to SYCLOS. OpenCL CPU RT has dependency on this PR.
I have submitted PR https://reviews.llvm.org/D142948 to LLORG. It's still under code review.

As the original code is used internally for OpenCL CPU RT, so we need this PR to be landed in SYCLOS even if https://reviews.llvm.org/D142948 is rejected by LLORG. It's urgent for us.

clang/lib/CodeGen/TargetInfo.cpp

premanandrao · 2023-02-01T17:53:41Z

clang/lib/CodeGen/TargetInfo.cpp

@@ -4427,6 +4469,10 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs,
 }

 void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const {


Can you extend the test below for this target as well?

triple x86_64-unknown-unknown in the test is already covering WinX86_64ABIInfo

Is that because the test is being run on Windows? Otherwise, I am not sure how it defaults to Windows and not say, for example, Linux.

Is that because the test is being run on Windows?

Yes. On linux, the test runs in X86_64ABIInfo::computeInfo path.

Okay, please add a Windows-specific triple to the test.

triple x86_64-unknown-unknown in the test is already covering WinX86_64ABIInfo

sorry this answer is incorrect. You're right that windows x86_64 target isn't covered in the first commit. I've added WinX86_64ABIInfo test in the new commit.

I also changed i686-unknown-unknown triple to i686-pc-win32-gnu, in order to match with the use scenario of this PR in OpenCL builtin build. DEVICE_TRIPLE is i686-pc-win32-gnu-elf and x86_64-pc-win32-gnu-elf for windows build in file backend/libraries/CMakeLists.txt

premanandrao · 2023-02-01T17:55:10Z

clang/test/CodeGenOpenCL/vector-to-scalar-coercion.cl

+
+// NOCOER:     define {{.*}}<4 x i64> @_Z18convert_ulong4_rteDv4_t(<4 x i16> noundef %{{.*}})
+// COER32CL:   define {{.*}}<4 x i64> @_Z18convert_ulong4_rteDv4_t(i64 noundef %{{.*}})
+// COER64CL:   define {{.*}}<4 x i64> @_Z18convert_ulong4_rteDv4_t(double noundef %{{.*}})


Is 'double' the right type here?

this is not related to this PR.

If we add ElementType->isSpecificBuiltinType(BuiltinType::UShort) to

llvm/clang/lib/CodeGen/TargetInfo.cpp

Line 2974 in aa69e4d

ElementType->isSpecificBuiltinType(BuiltinType::ULong)))

, the 'double' will be changed to 'i64'. Should we add signed/unsigned i8/i16/i32 types to that place?

I think we should, otherwise I am concerned the representation will be incorrect. Is something like isIntegerType or isIntegralOrEnumerationType() more appropriate there instead of checking each integral type kind there?

@erichkeane, could you please give your input here? I am really not sure.

ping @erichkeane . thanks

This 'double' type issue isn't related to this PR. @premanandrao could you please approve this PR if there is no other issues? This PR blocks the pre-commit build of #8216

Just got to the office after being away last week for the WG21 meeting. We definitely need to not make the element type here double, I can't imagine the optimizer is going to do good things with that/those conversions. I don't follow this well enough to know whether this cna be approved yet, so I'll leave that to Prem.

Thanks @erichkeane!
@hewj03, I think TargetInfo.cpp should be fixed to not have this converted to a double. Please create an issue to fix that in a separate PR if it won't be done as part of this. I don't think we should lose track of that issue. And add a FIXME to this test where we see the possibly incorrect 'double' types.

Please create an issue to fix that in a separate PR if it won't be done as part of this.

Issue #8347 is created for the 'double' type.

premanandrao · 2023-02-14T17:35:47Z

clang/lib/CodeGen/TargetInfo.cpp

@@ -4427,6 +4469,10 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs,
 }

 void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const {


Okay, please add a Windows-specific triple to the test.

premanandrao · 2023-02-14T17:49:43Z

clang/test/CodeGenOpenCL/vector-to-scalar-coercion.cl

+
+// NOCOER:     define {{.*}}<4 x i64> @_Z18convert_ulong4_rteDv4_t(<4 x i16> noundef %{{.*}})
+// COER32CL:   define {{.*}}<4 x i64> @_Z18convert_ulong4_rteDv4_t(i64 noundef %{{.*}})
+// COER64CL:   define {{.*}}<4 x i64> @_Z18convert_ulong4_rteDv4_t(double noundef %{{.*}})


Thanks @erichkeane!
@hewj03, I think TargetInfo.cpp should be fixed to not have this converted to a double. Please create an issue to fix that in a separate PR if it won't be done as part of this. I don't think we should lose track of that issue. And add a FIXME to this test where we see the possibly incorrect 'double' types.

clang/lib/CodeGen/TargetInfo.cpp

…in32-gnu

cdai2 · 2023-02-17T01:47:14Z

ping @intel/dpcpp-clang-driver-reviewers. Please review this PR.

hchilama

LGTM

cdai2 · 2023-02-17T06:53:31Z

@intel/llvm-gatekeepers Please help to merge this PR.

cdai2 · 2023-02-17T16:28:23Z

The LLVM test suites shown error info like "Error: Fail to load /localdisk2/github/_work/llvm/llvm/./llvm/devops/actions/llvm_test_suite/action.yml". Are there some issues in test infrastructure?

bader · 2023-02-17T16:35:38Z

The LLVM test suites shown error info like "Error: Fail to load /localdisk2/github/_work/llvm/llvm/./llvm/devops/actions/llvm_test_suite/action.yml". Are there some issues in test infrastructure?

@cdai2, please, update your branch. There should be no issues with the tip of the branch.

cdai2 · 2023-02-20T08:42:07Z

The LLVM test suites shown error info like "Error: Fail to load /localdisk2/github/_work/llvm/llvm/./llvm/devops/actions/llvm_test_suite/action.yml". Are there some issues in test infrastructure?

@cdai2, please, update your branch. There should be no issues with the tip of the branch.

Thanks a lot to rebase this PR. I have restarted the last failed Precommit test of RHEL build. It failed because of timeout.

cdai2 · 2023-02-20T16:05:50Z

@bader All checks have passed. thanks

For x86 target, vector types (both result and arguments) can be coerced to scalars of the same size, e.g: define zeroext i1 @_Z18convert_ulong4_rteDv4_t(<4 x i16> %x) ; becomes define zeroext i1 @_Z18convert_ulong4_rteDv4_t(i64 %x.coerced) Such behavior is completely valid for x86, but the backend vectorizer cannot work with scalars instead of vectors. With this patch, argument and result types will be leaved unchanged in the CodeGen. New option fopencl-force-vector-abi is also added to force-disables vector to scalar coercion when provided. --------- Co-authored-by: Wenju He <[email protected]> Co-authored-by: Alexey Bader <[email protected]>

cdai2 requested review from a team as code owners January 31, 2023 08:35

cdai2 requested review from romanovvlad and bader January 31, 2023 08:35

cdai2 temporarily deployed to aws January 31, 2023 09:22 — with GitHub Actions Inactive

Fznamznon reviewed Jan 31, 2023

View reviewed changes

cdai2 temporarily deployed to aws January 31, 2023 15:49 — with GitHub Actions Inactive

premanandrao reviewed Feb 1, 2023

View reviewed changes

FIx clang comment punctuation.

a04b937

cdai2 temporarily deployed to aws February 6, 2023 18:44 — with GitHub Actions Inactive

cdai2 temporarily deployed to aws February 6, 2023 20:21 — with GitHub Actions Inactive

wenju-he requested a review from premanandrao February 9, 2023 03:58

premanandrao reviewed Feb 14, 2023

View reviewed changes

add x86_64-pc-win32 test, add fixme, change 32bit triple to i686-pc-w…

30ecfa4

…in32-gnu

wenju-he mentioned this pull request Feb 15, 2023

[OpenCL][x86_64] clang/lib/CodeGen/TargetInfo.cpp: function argument type <4 x i16> is coerced to double #8347

Open

wenju-he requested a review from premanandrao February 15, 2023 08:57

wenju-he temporarily deployed to aws February 15, 2023 16:27 — with GitHub Actions Inactive

premanandrao approved these changes Feb 15, 2023

View reviewed changes

wenju-he temporarily deployed to aws February 16, 2023 00:08 — with GitHub Actions Inactive

hchilama approved these changes Feb 17, 2023

View reviewed changes

cdai2 requested a review from a team February 17, 2023 06:52

wenju-he temporarily deployed to aws February 17, 2023 15:46 — with GitHub Actions Inactive

wenju-he temporarily deployed to aws February 17, 2023 15:47 — with GitHub Actions Inactive

Merge branch 'sycl' into Disable_vector_to_scalar_types_coercion

d322d4a

bader temporarily deployed to aws February 18, 2023 20:14 — with GitHub Actions Inactive

bader temporarily deployed to aws February 18, 2023 20:54 — with GitHub Actions Inactive

bader merged commit 8b55761 into intel:sycl Feb 20, 2023

		@@ -4427,6 +4469,10 @@ ABIArgInfo WinX86_64ABIInfo::classify(QualType Ty, unsigned &FreeSSERegs,
		}

		void WinX86_64ABIInfo::computeInfo(CGFunctionInfo &FI) const {

[OpenCL] Disable vector to scalar types coercion for OpenCL #8160

[OpenCL] Disable vector to scalar types coercion for OpenCL #8160

Uh oh!

Conversation

cdai2 commented Jan 31, 2023

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

cdai2 commented Jan 31, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenju-he Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdai2 commented Feb 17, 2023

Uh oh!

hchilama left a comment

Choose a reason for hiding this comment

Uh oh!

cdai2 commented Feb 17, 2023

Uh oh!

cdai2 commented Feb 17, 2023

Uh oh!

bader commented Feb 17, 2023

Uh oh!

cdai2 commented Feb 20, 2023

Uh oh!

cdai2 commented Feb 20, 2023

Uh oh!

Uh oh!

wenju-he Feb 5, 2023 •

edited

Loading