This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Codegen of the ‘aligned’ clause for the ‘omp simd’ directive
ClosedPublic

Authored by amusman on Sep 26 2014, 2:34 AM.

Details

Summary

Please review codegen of the ‘aligned’ clause for the ‘omp simd’ directive.
It uses @llvm.assume intrinsic to specify alignment hints in LLVM IR.

Diff Detail

Repository
rL LLVM

Event Timeline

amusman updated this revision to Diff 14097.Sep 26 2014, 2:34 AM
amusman retitled this revision from to [OPENMP] Codegen of the ‘aligned’ clause for the ‘omp simd’ directive.
amusman updated this object.
amusman edited the test plan for this revision. (Show Details)
amusman added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Sep 26 2014, 10:26 AM
include/clang/Basic/DiagnosticSemaKinds.td
7180 ↗(On Diff #14097)

I don't like the " - ignored" part, how about rewording like this:

"aligned clause will be ignored because the requested alignment is not a power of 2"
lib/CodeGen/CGStmtOpenMP.cpp
53 ↗(On Diff #14097)

Remove this (see below).

76 ↗(On Diff #14097)

I don't think that using Safelen here is the right thing to do; a loop can have a very large safelen without that implying anything about the alignment of the pointers.

The right thing to do here is to use a callback into TargetCodeGenInfo (which you can get at using the TheTargetCodeGenInfo global).

105 ↗(On Diff #14097)

Remove the FIXME.

amusman updated this revision to Diff 14157.Sep 29 2014, 4:31 AM

Hi Hal,

Thank you for review, I've fixed your comments. Here is updated version.
I added method getOpenMPSimdDefaultAlignment to TheTargetCodeGenInfo (implemented it for x86_64 only for now).

Regards,
Alexander

hfinkel added inline comments.Sep 29 2014, 5:38 AM
lib/CodeGen/CGStmtOpenMP.cpp
64 ↗(On Diff #14157)

I think that you should just assert that this is either a power of 2 (or zero): if a target starts returning some other kind of number, that would be a bug, and we'd want to know.

69 ↗(On Diff #14157)

As I also mention below, when we use a default alignment, I'll need that to depend on the type, so you'll need to rearrange things here slightly.

94 ↗(On Diff #14157)

You don't need the {} here.

lib/CodeGen/TargetInfo.h
223 ↗(On Diff #14157)

I don't really like this description; how about this:

/// Gets the target-specific default alignment used when an 'aligned' clause is used with a 'simd' OpenMP directive without specifying a specific alignment.

224 ↗(On Diff #14157)

In general, I'll need this to depend on the type; please add a type parameter.

amusman updated this revision to Diff 14162.Sep 29 2014, 7:01 AM

Hal,

Here is updated patch, I added type parameter to getOpenMPSimdDefaultAlignment (I guess you meant llvm::Type, not clang::Type) and fixed other issues.

Thanks,
Alexander

hfinkel accepted this revision.Sep 29 2014, 10:30 AM
hfinkel edited edge metadata.

I added type parameter to getOpenMPSimdDefaultAlignment (I guess you meant llvm::Type, not clang::Type)

I think I'd actually like the QualType (which I suppose is just E->getType()), so that pointers to single-element structures can be handled like pointers to the underlying type in a straightforward way (meaning using the same logic that already exists in the ABI code).

Otherwise, LGTM. Thanks!

This revision is now accepted and ready to land.Sep 29 2014, 10:30 AM
amusman closed this revision.Sep 29 2014, 10:40 PM
amusman updated this revision to Diff 14203.

Closed by commit rL218660 (authored by @amusman).