Page MenuHomePhabricator

[PowerPC] [Clang] Port MMX intrinsics and basic test cases to Power
ClosedPublic

Authored by qiucf on Mar 28 2019, 2:36 AM.

Details

Summary

Port existing headers which include x86 intrinsics implementation to PowerPC platform (using Altivec).

Since x86 intrinsic headers (like mmintrin.h) are already at clang's header directory, PowerPC's toolchain class is overrided to insert new headers directory (named ppc_wrappers) into the path. Test cases for several intrinsic functions are added as an example.

Only mmintrin.h (MMX instruction set intrinsics) are added in this patch. The header, along with others, is mainly developed by Steven Munroe, with contributions from Paul Clarke, Bill Schmidt, Jinsong Ji and Zixuan Wu.

Diff Detail

Repository
rC Clang

Event Timeline

qiucf created this revision.Mar 28 2019, 2:36 AM
jsji requested changes to this revision.Apr 8 2019, 8:09 AM

Since we are adding new headers that is not originated by this patch,
please also update the description (and commit message) to include who actually contributed to mmintrin.h and other headers.
eg:headers are mostly based on headers developed by Steven Munroe, with some contribution of Paul Clarke, Bill Schmidt, Jinsong, Zeson.

clang/lib/Driver/ToolChains/PPCLinux.cpp
23

Why we want to exclude these includes when nostdinc is on? These include files are NOT standard include files.

clang/lib/Driver/ToolChains/PPCLinux.h
31

missing end in comment?

clang/test/CodeGen/ppc-mmintrin.c
25

We should use CHECK-NEXT instead of CHECK here. This also apply to most of the following CHECK.

26

Why we omit checking two load for vec_packs here?

37

We should also check the load and make sure that the endian-specific code are working.

clang/test/Headers/ppc-intrinsics.c
4

Why -DTEST_MACRO?

8

NOT?
NOT: command not found ^

This revision now requires changes to proceed.Apr 8 2019, 8:09 AM
qiucf marked 2 inline comments as done.Apr 8 2019, 11:34 PM
qiucf added inline comments.
clang/lib/Driver/ToolChains/PPCLinux.cpp
23

-nostdinc: Do not search the standard system directories or compiler builtin directories for include files.
From Clang's man page

And on Windows toolchain (MSVC.cpp), if turning on -nostdinc, Windows SDK files will also not be added into path.

clang/test/Headers/ppc-intrinsics.c
8

It should be not, in downcase. This is what other test cases (see test/CXX/temp/temp.spec/no-body.cpp) use, to assert a non-zero exit code of a command. Otherwise, llvm-lit would complain and give a failure.

qiucf updated this revision to Diff 194253.Apr 9 2019, 12:03 AM
qiucf retitled this revision from [PowerPC][Clang] Port MMX intrinsics and basic test cases to Power to [PowerPC] [Clang] Port MMX intrinsics and basic test cases to Power.
qiucf edited the summary of this revision. (Show Details)
  • Add more check of mmintrin generated IR against endianness.
  • Fix typo in PPCLinux header.
  • Fix command error in ppc intrinsics header test.
qiucf edited the summary of this revision. (Show Details)Apr 9 2019, 12:03 AM
qiucf marked 6 inline comments as done.
jsji requested changes to this revision.Apr 9 2019, 1:02 PM

Please make sure you run all test before updating patch!

clang/test/CodeGen/ppc-mmintrin.c
24

.../clang/test/CodeGen/ppc-mmintrin.c:23:11: error: CHECK: expected string not found in input
// CHECK: store i64 [[REG1]], i64* [[REG3:[0-9a-zA-Z_%.]+]], align 8

This revision now requires changes to proceed.Apr 9 2019, 1:02 PM
qiucf updated this revision to Diff 194446.Apr 9 2019, 10:53 PM

Fix error about a mismatch in mmintrin test cases.

qiucf marked an inline comment as done.Apr 9 2019, 11:47 PM

Part of a function signature can also match where it is called, so the test failed. I did the changes to pass the test but diff file was generated locally so it didn't contain this change. Sorry for the mistake.

jsji accepted this revision.Apr 15 2019, 12:34 PM

LGTM. Thanks for porting!

This revision is now accepted and ready to land.Apr 15 2019, 12:34 PM
This revision was automatically updated to reflect the committed changes.

You use %clang (clang driver) because the test needs -internal-isystem $resource_dir/include/ppc_wrappers. A default release build of llvm sets LLVM_ENABLE_ASSERTIONS to off, the clang driver defaults to -fdiscard-value-names. This caused the test to break. I have fixed that in rC358953.

May I ask why the mmintrin.h emulation layer must be added to the clang resource directory? Why can't it be provided as a standalone library like https://github.com/intel/ARM_NEON_2_x86_SSE ? Will you add SSE/AVX/AVX512 emulation layer to the clang resource directory as follow-ups?

MaskRay added inline comments.Apr 23 2019, 6:14 AM
lib/Headers/ppc_wrappers/mmintrin.h
3 ↗(On Diff #196182)

This is not Apache 2.

24 ↗(On Diff #196182)

Is this considered a derivative work of the Intel C++ Compiler?

jsji added a comment.Apr 23 2019, 7:44 AM

May I ask why the mmintrin.h emulation layer must be added to the clang resource directory? Why can't it be provided as a standalone library like https://github.com/intel/ARM_NEON_2_x86_SSE ?

The intention is to lower the porting effort for users, so that user don't need to modify their source or add their own additional headers wrapper layer.

Will you add SSE/AVX/AVX512 emulation layer to the clang resource directory as follow-ups?

Yes, more headers (eg: xmmintrin.h, emmintrin.h) will follow.

jsji added a comment.Apr 23 2019, 7:58 AM

Thanks @MaskRay for help fixing and review!

lib/Headers/ppc_wrappers/mmintrin.h
3 ↗(On Diff #196182)

Ah.. This was aligned with other headers .. but forgot to update to Apache 2.

@qiucf Please update it to Apache 2 as well. Thanks.

24 ↗(On Diff #196182)

My understanding is that the User Guide and Reference is the specification , implementation based on public specification should NOT be considered as derivative work.

Otherwise, all codes that referenced User Guide and Reference need to be considered derivative work?

@chandlerc, I hate to do this to you, but licensing question.

jsji added inline comments.Apr 30 2019, 7:21 PM
lib/Headers/ppc_wrappers/mmintrin.h
3 ↗(On Diff #196182)

Updated to Apache 2 in https://reviews.llvm.org/rC359164