Fix Clang compilation error with Lib ATen for ppc64le #106446

pratiklp00 · 2023-08-02T06:22:16Z

This patch fixes error while compiling with Clang for ppc64le
I have used clang version 15.0.7
Errors are as follow:

No matching function for call to 'vec_sel’
No matching function for call to 'vec_splats'
Excess elements in scalar initializer
Use of undeclared identifier 'vec_vsubudm'
Fix for multiple error within int64_t  DEFINE_MEMBER_OP_AND_ONE

References:

cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10

pytorch-bot · 2023-08-02T06:22:18Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/106446

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 5e03048 with merge base acd595e ():

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

pull / linux-focal-py3_8-clang9-xla / test (xla, 1, 1, linux.12xlarge, unstable) (gh)

This comment was automatically generated by Dr. CI and updates every 15 minutes.

linux-foundation-easycla · 2023-08-02T06:22:19Z

The committers listed above are authorized under a signed CLA.

✅ login: pratiklp00 / name: Pratik (3310915, 166d46f, 499f13f)
✅ login: malfet / name: Nikita Shulga (2c9602d, 8b75f3d, 5e03048)

pratiklp00 · 2023-08-17T09:32:36Z

@malfet @XiaobingSuper Hi, please let me know if there is any suggestion or change/comment

malfet

Few remakrs:

Please specify clang version in PR description and reference some doc explaining why __attribute((altivec(vector__)) does not work for clang, but works for gcc , and vice versa (i.e. if say gcc-9+ supports __attribute__((vector_size(16)) as alternative to altivec attribute, perhaps entire code can be changed to it
Please explain why vint64 needs to be defined as signed long long rather than signed long or even int64_t? (This would also avoid force casting vec_splats, wouldn't it?)

malfet · 2023-08-17T20:53:02Z

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h

+#if defined(__clang__)
+#if !defined(vec_splats)


Is there a reason not to combine the two?

Suggested change

#if defined(__clang__)

#if !defined(vec_splats)

#if defined(__clang__) && !defined(vec_splats)

No, I will combine it.

malfet · 2023-08-17T20:54:59Z

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h

+#if defined(__clang__)
+#if !defined(vec_splats)
+C10_ALWAYS_INLINE vint64 vec_splats(const int64_t& a) {
+ return vec_splats(static_cast<signed long long>(a));


Please check that int64_t and signed long long are different types, otherwise it feels like a tail recursion.

Suggested change

return vec_splats(static_cast<signed long long>(a));

static_assert(!std::is_same_v<int64_t, signed long long>, "Cast is meaningless here");

return vec_splats(static_cast<signed long long>(a));

No it is not different. I will remove the casting.

malfet · 2023-08-17T20:57:41Z

aten/src/ATen/cpu/vec/vec256/vsx/vec256_int64_vsx.h

@@ -37,7 +38,7 @@ class Vectorized<int64_t> {
  C10_ALWAYS_INLINE Vectorized(vint64 v1, vint64 v2) : _vec0{v1}, _vec1{v2} {}
  C10_ALWAYS_INLINE Vectorized(vbool64 v1, vbool64 v2) : _vecb0{v1}, _vecb1{v2} {}
  C10_ALWAYS_INLINE Vectorized(int64_t scalar)
-      : _vec0{vec_splats(scalar)}, _vec1{vec_splats(scalar)} {}
+      : _vec0{vec_splats((ElementType)scalar)}, _vec1{(vint64)vec_splats((ElementType)scalar)} {}


Those casts feel unnecessary after you've defined an overload in vsx_helpers.h aren't they?
In that case, please remove them

I have removed them.

vinithakv · 2023-10-09T06:47:16Z

@pratiklp00
Some extra spaces reported by linter. Otherwise looks good to me.

pratiklp00 · 2023-10-23T03:44:01Z

@malfet Hi, I have made the required changes Please review it.

pratiklp00 · 2023-11-09T02:13:34Z

@malfet @ezyang @ezyang Hi Please let me know if any changes are required.

malfet · 2023-11-09T14:23:21Z

@pratiklp00 at the very list current form of the change break CI all over the place, can you please fix it?

malfet · 2023-11-09T14:23:38Z

@pytorchbot rebase

pytorchmergebot · 2023-11-09T14:25:18Z

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

pytorchmergebot · 2023-11-09T14:25:23Z

Successfully rebased libATenFix onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout libATenFix && git pull --rebase)

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h

malfet · 2023-11-09T14:28:19Z

@pytorchbot merge

pytorchmergebot · 2023-11-09T14:30:19Z

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team

Raised by workflow job

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h

malfet · 2023-11-09T17:25:15Z

@pytorchbot merge -f "Lint is green"

pytorchmergebot · 2023-11-09T17:27:03Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

This patch fixes error while compiling with Clang for ppc64le I have used clang version 15.0.7 Errors are as follow: ``` No matching function for call to 'vec_sel’ No matching function for call to 'vec_splats' Excess elements in scalar initializer Use of undeclared identifier 'vec_vsubudm' Fix for multiple error within int64_t DEFINE_MEMBER_OP_AND_ONE ``` References: - https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html - https://reviews.llvm.org/D81083 Co-authored-by: Nikita Shulga <2453524+malfet@users.noreply.github.com> Pull Request resolved: pytorch#106446 Approved by: https://github.com/malfet

github-actions bot added the module: cpu CPU specific problem (e.g., perf, algorithm) label Aug 2, 2023

pytorchbot added the open source label Aug 2, 2023

ezyang requested a review from malfet August 2, 2023 13:37

ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 2, 2023

ezyang requested a review from XiaobingSuper August 2, 2023 13:37

malfet requested changes Aug 17, 2023

View reviewed changes

pratiklp00 added 3 commits November 9, 2023 14:25

fix clang compilation error

3310915

chnages suggested by malfet

166d46f

chnages suggested by malfet

499f13f

pytorchmergebot force-pushed the libATenFix branch from 5a1c00d to 499f13f Compare November 9, 2023 14:25

malfet reviewed Nov 9, 2023

View reviewed changes

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h Outdated Show resolved Hide resolved

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h Outdated Show resolved Hide resolved

Apply suggestions from code review

2c9602d

malfet approved these changes Nov 9, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 9, 2023

pytorchmergebot added the merging label Nov 9, 2023

pytorchmergebot removed the merging label Nov 9, 2023

malfet reviewed Nov 9, 2023

View reviewed changes

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h Outdated Show resolved Hide resolved

malfet reviewed Nov 9, 2023

View reviewed changes

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h Outdated Show resolved Hide resolved

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h Outdated Show resolved Hide resolved

Apply suggestions from code review

8b75f3d

malfet reviewed Nov 9, 2023

View reviewed changes

aten/src/ATen/cpu/vec/vec256/vsx/vec256_complex_float_vsx.h Outdated Show resolved Hide resolved

aten/src/ATen/cpu/vec/vec256/vsx/vsx_helpers.h Outdated Show resolved Hide resolved

Apply suggestions from code review

5e03048

malfet added release notes: build release notes category topic: bug fixes topic category labels Nov 9, 2023

pytorchmergebot added the merging label Nov 9, 2023

pytorchmergebot added Merged and removed merging labels Nov 9, 2023

pytorchmergebot closed this in 4da5d4b Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Clang compilation error with Lib ATen for ppc64le #106446

Fix Clang compilation error with Lib ATen for ppc64le #106446

pratiklp00 commented Aug 2, 2023 •

edited by malfet

pytorch-bot bot commented Aug 2, 2023 •

edited

linux-foundation-easycla bot commented Aug 2, 2023 •

edited

pratiklp00 commented Aug 17, 2023

malfet left a comment

malfet Aug 17, 2023

pratiklp00 Sep 20, 2023

malfet Aug 17, 2023

pratiklp00 Sep 20, 2023

malfet Aug 17, 2023

pratiklp00 Sep 20, 2023

vinithakv commented Oct 9, 2023

pratiklp00 commented Oct 23, 2023

pratiklp00 commented Nov 9, 2023

malfet commented Nov 9, 2023

malfet commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

malfet commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

malfet commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

	#if defined(__clang__)
	#if !defined(vec_splats)
	#if defined(__clang__) && !defined(vec_splats)

	return vec_splats(static_cast<signed long long>(a));
	static_assert(!std::is_same_v<int64_t, signed long long>, "Cast is meaningless here");
	return vec_splats(static_cast<signed long long>(a));

Fix Clang compilation error with Lib ATen for ppc64le #106446

Fix Clang compilation error with Lib ATen for ppc64le #106446

Conversation

pratiklp00 commented Aug 2, 2023 • edited by malfet

pytorch-bot bot commented Aug 2, 2023 • edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/106446

✅ You can merge normally! (1 Unrelated Failure)

linux-foundation-easycla bot commented Aug 2, 2023 • edited

pratiklp00 commented Aug 17, 2023

malfet left a comment

Choose a reason for hiding this comment

malfet Aug 17, 2023

Choose a reason for hiding this comment

pratiklp00 Sep 20, 2023

Choose a reason for hiding this comment

malfet Aug 17, 2023

Choose a reason for hiding this comment

pratiklp00 Sep 20, 2023

Choose a reason for hiding this comment

malfet Aug 17, 2023

Choose a reason for hiding this comment

pratiklp00 Sep 20, 2023

Choose a reason for hiding this comment

vinithakv commented Oct 9, 2023

pratiklp00 commented Oct 23, 2023

pratiklp00 commented Nov 9, 2023

malfet commented Nov 9, 2023

malfet commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

malfet commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

Merge failed

malfet commented Nov 9, 2023

pytorchmergebot commented Nov 9, 2023

Merge started

pratiklp00 commented Aug 2, 2023 •

edited by malfet

pytorch-bot bot commented Aug 2, 2023 •

edited

linux-foundation-easycla bot commented Aug 2, 2023 •

edited