This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OMPIRBuilder] Add generation of SIMD align assumptions to OMPIRBuilder
ClosedPublic

Authored by domada on Sep 9 2022, 7:19 AM.

Details

Summary

Currently generation of align assumptions for OpenMP simd construct is done outside OMPIRBuilder for C code and it is not supported for Fortran.

According to OpenMP 5.0 standard (2.9.3) only pointers and arrays can be aligned for C code.

If given aligned variable is pointer, then Clang generates the following set of the LLVM IR isntructions to support simd align clause:

; memory allocation for pointer address:
%A.addr = alloca ptr, align 8
; some LLVM IR code
; Alignment instructions (alignment is equal to 32):
%0 = load ptr, ptr %A.addr, align 8
call void @llvm.assume(i1 true) [ "align"(ptr %0, i64 32) ]

If given aligned variable is array, then Clang generates the following set of the LLVM IR isntructions to support simd align clause:

; memory allocation for array:
%B = alloca [10 x i32], align 16
; some LLVM IR code
; Alignment instructions (alignment is equal to 32):
%arraydecay = getelementptr inbounds [10 x i32], ptr %B, i64 0, i64 0
call void @llvm.assume(i1 true) [ "align"(ptr %arraydecay, i64 32) ]

OMPIRBuilder was modified to generate aligned assumptions. It generates only llvm.assume calls. Frontend is responsible for generation of aligned pointer and getting the default alignment value if user does not specify it in aligned clause.

Unit and regression tests were added to check if aligned clause was handled correctly.

; memory allocation for pointer address:
%A.addr = alloca ptr, align 8
; some LLVM IR code
; Alignment instructions (alignment is equal to 32):
%0 = load ptr, ptr %A.addr, align 8
call void @llvm.assume(i1 true) [ "align"(ptr %0, i64 32) ]

If given aligned variable is array, then Clang generates the following set of the LLVM IR instructions to support simd align clause:

; memory allocation for array: 
%B = alloca [10 x i32], align 16
; some LLVM IR code
; Alignment instructions (alignment is equal to 32):
%arraydecay = getelementptr inbounds [10 x i32], ptr %B, i64 0, i64 0
call void @llvm.assume(i1 true) [ "align"(ptr %arraydecay, i64 32) ]

OMPIRBuilder was modified to support this codegen scheme. Attached unit tests check if generation of align assumptions is successful.

Diff Detail

Event Timeline

domada created this revision.Sep 9 2022, 7:19 AM
Herald added a project: Restricted Project. · View Herald Transcript
domada requested review of this revision.Sep 9 2022, 7:19 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
domada added a project: Restricted Project.

Thanks for this patch! I have two drive by comments that should probably be addressed first.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
629

Aren't there different alignments possible, so X is aligned 32 and Y is aligned 64? If so, should we tie the Value and Alignment together in the API?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

This doesn't work. Use the data layout for any default values please.

domada updated this revision to Diff 459697.Sep 13 2022, 3:10 AM

Applied review remarks. Scope of changes:

  1. replaced ArrayRef<Value*> AlignedVars, Value * Alignment arguments with DenseMap. It allows to generate different alignment assumption for different variables. OpenMP allows to specify multiple aligned clauses for single SIMD construct.
  2. modified calculation of default alignment. Use pointer ABI alignment as the default one.
domada marked an inline comment as done.Sep 13 2022, 3:41 AM
domada added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
629

You are right. It is possible to specify multiple align clauses for single omp clause (for example: #pragma omp simd aligned(X:32) aligned(Y:64)

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

I have used pointer ABI alignment as the default value. Is it ok?
Clang has separate method to calculate OpenMP default alignment defined in TargetInfo class ( link)
Unfortunately, there is no simple replacement of this function in OMPIRBuilder or in Flang driver. There are some requests to expose TargetInfo data in Clang-independent manner: RFC1, RFC2 , but this issue is not solved.

domada updated this revision to Diff 464640.Oct 3 2022, 2:51 AM
domada marked an inline comment as done.
domada retitled this revision from [OpenMP] Add generation of SIMD align assumptions to OMPIRBuilder to [OpenMP][OMPIRBuilder] Add generation of SIMD align assumptions to OMPIRBuilder.

Patch rebased.

jdoerfert added inline comments.Oct 3 2022, 6:41 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

Let's step back:

  1. User tells us the alignment via aligned(X:64). This should populate the map in Clang, I would assume.
  2. User tells us the alignment "implicitly" via align(Y). Again, clang should populate the map with the default/preferred alignment of the type. You can use the function you linked, or use the data layout API to get the preferred alignment.
  3. User tells us nothing. We should pick the default or, don't annotate anything. If we go with defaults, you need to look up the default for the address space but I don't know why we would annotate anything not provided by the user.
domada added inline comments.Oct 3 2022, 7:46 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

Ad 1) Agree
Ad 2) OK, I will add assertion to ensure that the map is correctly populated with the default/preferred alignment if the user specifies only: align(Y).

I was thinking that OMPIRBuilder should pick the default value if the user specifies align(Y). Thanks for clarification.

Ad 3) I assume that if the user tells us nothing we should not annotate anything. I will expect empty map and no generated annotations. Currently we do not generate any annotations if there is no align clause.

domada updated this revision to Diff 464924.Oct 4 2022, 2:25 AM

Add assert to ensure that alignment value is always specified.

domada marked an inline comment as done.Oct 4 2022, 2:25 AM
jdoerfert added inline comments.Oct 4 2022, 5:36 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

I was thinking that OMPIRBuilder should pick the default value if the user specifies align(Y). Thanks for clarification.

That works with me, but you need the actual type. So the Value is not sufficient, you need &X and double to ask DataLayout for the preferred type. For now you can use the clang functionality or pass the type and use DL.

I assume that if the user tells us nothing we should not annotate anything.

Agreed.

domada updated this revision to Diff 465332.Oct 5 2022, 3:51 AM

Patch rebased.

domada added inline comments.Oct 5 2022, 4:37 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

I was thinking that OMPIRBuilder should pick the default value if the user specifies align(Y). Thanks for clarification.

That works with me, but you need the actual type. So the Value is not sufficient, you need &X and double to ask DataLayout for the preferred type. For now you can use the clang functionality or pass the type and use DL.

I rely on Clang functionality and I assume that the Clang always sets the size(default or specified by the user). That's why I added additional assert to applySimd function to be sure that Clang always sets the alignment value.

Currently Clang does not use the actual type to calculate the default simd alignment: https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 . IMO it's better to rely on Clang and it's functionality which is widely tested.

jdoerfert added inline comments.Oct 5 2022, 6:12 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2639

Where is this map populated?

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
628

The map of the variables which need to aligned with.

This reads weird. Can you rephrase

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2976

Currently Clang does not use the actual type to calculate the default simd alignment: https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 . IMO it's better to rely on Clang and it's functionality which is widely tested.

You basically say clang is using a sub-optimal choice, let's rely on that as it is tested. I'm fine with that, but it seems odd.

2995

Why use Value as type and then assert it's an Alloca? Use a map with Alloca as key.

2999

This won't be necessary once you change the type but as a hint, cast is the right choice after an isa.

3011

Is Alloca of an array tested? I would not have assumed two different paths here. GEP on the alloca makes an assumption about the alloca, load of the alloca makes an assumption about the allocated type. If anything, this distinction should be based on the "isArray" property, not the allocated type. I mean, alloca i32, 4 and alloca [4xi32] are different beasts. I fear we apply the handling of the former to the latter but I didn't check.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1965
1966
1967
1978

This doesn't actually test that 2 assumes with alignments have been found, one for each alloca.

domada updated this revision to Diff 466766.Oct 11 2022, 4:41 AM
domada edited the summary of this revision. (Show Details)
  1. Modified generation of align assumptions. OMPIRBuilder generates now only assumptions calls. The arguments of the assumption calls are generated by Clang.
  2. Added integration test to prove that Clang and OMPIRBuilder support aligned clause
  3. Simplification of unit tests -> applying review remarks
domada marked an inline comment as done.Oct 11 2022, 4:59 AM
domada added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
2639

I added function GetAlignedMapping which collects arguments for assumption calls. Now Clang collects all arguments required for assumption calls. OMPIRBuilder only inserts assumption calls.

When I was adding C integration tests I realized that it would be easier to implement in that way. Please let me know if this approach is ok for you.

domada updated this revision to Diff 466774.Oct 11 2022, 5:04 AM
domada marked an inline comment as done.
  1. Update description of AlignedVars argument
domada marked an inline comment as done.Oct 11 2022, 5:08 AM
domada added inline comments.
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1978

The latest change moved alloca handling to the Clang. That's why it is not necessary to check it here.

This now looks good. The test is the only things I don't understand.

clang/test/OpenMP/irbuilder_simd_aligned.cpp
69 ↗(On Diff #466774)

Did you not check the alignment here on purpose? I'd assume this is the most important line in the test, no? Also, consider this cannot be auto-updated anymore w/o loosing manual updates.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
638

No llvm::

domada updated this revision to Diff 468134.Oct 17 2022, 1:40 AM
  1. Changed DenseMap to MapVector. MapVector provides that the iteration order is the same as insertion order
domada marked 2 inline comments as done.Oct 17 2022, 3:28 AM
domada added inline comments.
clang/test/OpenMP/irbuilder_simd_aligned.cpp
69 ↗(On Diff #466774)

Yes, I replaced CHECK-NEXT by CHECK-COUNT because DenseMap does not provide reproducible order of iteration and we cannot assume that llvm.assume calls will be done in given order.

I decided to replace DenseMap with MapVector as the follow-up after your review. MapVector provides access to stored elements in deterministic order and we can rely on generated checks.

jdoerfert accepted this revision.Oct 17 2022, 9:09 AM

LG, thanks for working on this.

This revision is now accepted and ready to land.Oct 17 2022, 9:09 AM
This revision was automatically updated to reflect the committed changes.
domada marked an inline comment as done.