Page MenuHomePhabricator

[OpenMP] "declare simd" for AArch64 Advanced SIMD.
Changes PlannedPublic

Authored by fpetrogalli on Mar 8 2017, 8:15 AM.

Details

Summary

This patch enables the code generation of vector function names that are
described by attaching a "#pragma omp declare simd" directive to the
scalar function definition/declaration, for the Advanced SIMD (NEON)
vector extension of the A64 instruction set for the AArch64 execution
state of the ARMv8 architecture.

As it is done for other targets, the available vector functions are
stored as string attributes attached to the scalar function in the IR,
and are made available for further processing in the middle
end (e.g. exposure to the loop vectorizer).

The mangling function of the vector names is compatible with
the Itanium-standard names that are being generated for X86.

The value of the token that specifies the architecture extension is 'n',
as for NEON.

Changes

  • The SimdDefaultAlign value of the AArch64TargetInfo class has been set to 128-bit - the size of a quad-word register "Q" for NEON.
  • The name mangling function for X86 has been merged into a generic one that is shared with AArch64, as emitTargetDeclareSimdFunction.
  • The CodeGen test has been split into X86 and AArch64 runs.
  • To improve readability and maintainability, the actual FileCheck checks (both for X86 and AArch64) have been moved right after the respective function declarations.

Note

The patch does not introduce any functional change for the X86 target.

Diff Detail

Event Timeline

fpetrogalli created this revision.Mar 8 2017, 8:15 AM
fpetrogalli edited the summary of this revision. (Show Details)Mar 8 2017, 8:19 AM
fhahn added a subscriber: fhahn.Mar 8 2017, 8:39 AM
fhahn added inline comments.Mar 8 2017, 8:41 AM
lib/CodeGen/CGOpenMPRuntime.cpp
6664

Shouldn't this indentation be on the same level as namespace{}?

fpetrogalli updated this revision to Diff 91035.Mar 8 2017, 9:32 AM
fpetrogalli marked an inline comment as done.

Changes:

  • fixed formatting;
  • added two tests that were missing.

Gentle ping.

Thanks,

Francesco

Looks ok to me, but I'm not very knowledgeable in that area. Hopefully some OpenMP Clang developers I added could give you a more concrete approval. :)

lib/CodeGen/CGOpenMPRuntime.cpp
6821

No f32?

fpetrogalli added inline comments.Mar 15 2017, 6:46 AM
lib/CodeGen/CGOpenMPRuntime.cpp
6821

Sorry, I am not sure to understand what you mean here by f32.

I am considering the double-word registers (64 bits wide) and quad-word registers (128 bits wide) that are used in AdvSIMD (NEON) for AArch64 [1].

[1] https://developer.arm.com/docs/dui0473/latest/neon-programming/neon-views-of-the-extension-register-bank

rengolin added inline comments.Mar 15 2017, 6:50 AM
lib/CodeGen/CGOpenMPRuntime.cpp
6821

Sorry, I meant S0~Sn, but that's silly, as it's VFP. Ignore me.

Hahnfeld edited edge metadata.Mar 21 2017, 5:48 AM

In principal looks good to me although I'm not really familiar with this part. Does that work for you if you have the declare simd in a header file and the implementation in another file? On x86_64 I currently get:

remark: loop not vectorized: call instruction cannot be vectorized

But that seems to be a general problem inside LLVM's LoopVectorize pass...

In principal looks good to me although I'm not really familiar with this part. Does that work for you if you have the declare simd in a header file and the implementation in another file? On x86_64 I currently get:

The current infrastructure for vector names generation works only for function definition. Ideally we should implement it also for function definition provided by external libraries.

remark: loop not vectorized: call instruction cannot be vectorized

But that seems to be a general problem inside LLVM's LoopVectorize pass...

AFAIK, none of the machinery required in LLVM to expose the mangled names in the vectorizer is present in trunk. There is a patch for x86 under review that does that: https://reviews.llvm.org/D22792

Hahnfeld accepted this revision.Mar 21 2017, 6:18 AM

AFAIK, none of the machinery required in LLVM to expose the mangled names in the vectorizer is present in trunk. There is a patch for x86 under review that does that: https://reviews.llvm.org/D22792

Thanks for the link. I think this patch is fine in that case.

This revision is now accepted and ready to land.Mar 21 2017, 6:18 AM

Thanks, I do not have commit rights, can anyone commit this?

ABataev edited edge metadata.Mar 21 2017, 10:04 AM

Guys, sorry for the delay with the reviews. I was on a vacation, now I'm on a relocation process. Will look at all patches as soon as possible, but not earlier than next week :( Sorry again.

fpetrogalli planned changes to this revision.Mar 23 2017, 10:29 AM

Dear all,

thank you for reviewing this patch. We found out an error in the spec and would like to fix it before things are used.

I will soon update this patch.

Thanks,

Francesco

ABataev added inline comments.Apr 6 2017, 12:51 PM
lib/CodeGen/CGOpenMPRuntime.cpp
6818–6819

else if

6826

Maybe it is better to create SmallVector<ISADataTy, 4> ISAData and fill it for all architectures independently, but have just one call of emitTargetDeclareSimdFunction()?

Alexey,

thank you for reviewing this. If you don't mind, I will wait applying the changes you requested, Unfortunately I am not ready yet to send out an update as I am still working on the vector ABI. I will ping you when ready.

Francesco

Hahnfeld resigned from this revision.Feb 17 2018, 2:00 AM