Page MenuHomePhabricator

[mlir][OpenMP] Add aligned clause definition to simd construct
ClosedPublic

Authored by domada on Oct 13 2022, 3:38 AM.

Details

Summary

simd aligned construct is represented as pair of variable which needs to be aligned and corresponding alignment value.

Added parser, printer and verifier of aligned clause. MLIR tests were updated to test correctness of MLIR definition of aligned clause.

Diff Detail

Event Timeline

domada created this revision.Oct 13 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
domada requested review of this revision.Oct 13 2022, 3:38 AM
kiranchandramohan requested changes to this revision.Oct 13 2022, 4:52 AM

Thanks for this patch @domada. Have a few questions/comments.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
389–391

Can you add this to a verifier?

415

If this is a variable then it is best to have the type OpenMP_PointerLikeType.

416

Does the standard specify I32?
Also, if this is a constant then it should be an attribute, something like

ConfinedAttr<OptionalAttr<I64Attr>, [IntMinValue<0>]>:$aligned_val,
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
145

OpAsmParser has a parseCommaSeparatedList, can that be used instead?

mlir/test/Dialect/OpenMP/ops.mlir
383

Add a test with a list.

This revision now requires changes to proceed.Oct 13 2022, 4:52 AM
domada updated this revision to Diff 469168.Oct 20 2022, 5:11 AM
domada edited the summary of this revision. (Show Details)

Addressed review comments:

  1. Modified the way how aligned clause is represented. Use I64ArrayAttr instead of Variadic to represent alignment values
  2. Added verifier which checks if aligned clause was properly mapped
  3. Added more tests MLIR tests
domada marked 4 inline comments as done.Oct 20 2022, 5:24 AM
domada added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
416

OpenMP standard does not specify the size of alignment.

OpenMP standard allows to use multiple aligned clause to specify the alignment of different variables. That's why I needed to use an I64ArrayAttr to specify the alignment of each variable. I added to the verifier checks for positivity of every array element.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
145

The format of alignment has changed. It is not variadic list any more. That's why I leave linear clauses unmodified.

I also use parseCommaSeparatedList to parse aligned clause.

domada marked 2 inline comments as done.Oct 20 2022, 5:58 AM
domada retitled this revision from [mlir][OpenMP] Add aligned clause defintion to simd construct to [mlir][OpenMP] Add aligned clause definition to simd construct.Oct 20 2022, 6:38 AM
kiranchandramohan requested changes to this revision.Oct 29 2022, 3:31 AM

Looks mostly fine. Please add tests for all verification errors.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
179

Nit:

186

Nit: Can you add a test for this?

194

Nit: Can you add a test for this?
For now, this is probably fine. But it is probably possible that there are multiple pointer vals pointing to the same address and this check will not catch it.

205

Nit: Can you add a test for this?

This revision now requires changes to proceed.Oct 29 2022, 3:31 AM
domada updated this revision to Diff 471969.Oct 31 2022, 5:01 AM

Applied remarks. Added more tests.

domada marked 4 inline comments as done.Oct 31 2022, 5:34 AM
domada added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
186

Added test case: omp_simdloop_unexpected_alignment

194

Added test case:
omp_simdloop_aligned_the_same_var

I checked that clang does not inform the user that two items point to the same address. The code given below: compiles without any warning: (build command: trunk_build/clang -fopenmp -c code_below.c )
int foo(int *A, int *B) {
int *p = B;
#pragma omp simd aligned(B) aligned(A, p :256)
for (int i = 0; i< 10; ++i) {
A[i] = i+44;
B[i] = i+2 + *p;
}
return 0;
}

I agree that we need to warn user about such buggy code, but in my opinion we need to add pointer aliasing analysis before OpenMP MLIR verification. I think that this issue should be addressed by different patch. Currently I can add TODO comment to remember about pointer aliaising.

205

Added test case:
omp_simdloop_aligned_float .
Alignment values are modelled as I64ArrayAttr in MLIR dialect. That's why I was not able to trigger "expected integer alignment" error. I am able to trigger generic MLIR parser error which corresponds to parsing error of I64ArrayAttr .

domada marked 3 inline comments as done.Oct 31 2022, 5:44 AM

LGTM.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
194

OK, that is fine. No need to do anything heavy-weight for now.

This revision is now accepted and ready to land.Oct 31 2022, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2022, 9:10 AM