This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP 5.0] Fix user-defined mapper lookup in sema
ClosedPublic

Authored by lildmh on Sep 24 2019, 12:57 PM.

Details

Summary

This patches fixes the case when a user-defined mapper is attached to the elements of an array, and to report error when a mapper is used for types other than struct, class, and union.

Diff Detail

Repository
rL LLVM

Event Timeline

lildmh created this revision.Sep 24 2019, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 12:57 PM

What happens without this patch?

Without this patch, it cannot recognize array with mapper, for instance, #pragma omp target map(mapper(a),to: arr[0:2]) won't work without this patch.

Without this patch, it cannot recognize array with mapper, for instance, #pragma omp target map(mapper(a),to: arr[0:2]) won't work without this patch.

What if we have a mapper for the array type?

Without this patch, it cannot recognize array with mapper, for instance, #pragma omp target map(mapper(a),to: arr[0:2]) won't work without this patch.

What if we have a mapper for the array type?

Without this patch, it won't be able to find the mapper in this case, and Clang assumes no mapper. This patch fixes it, so the compiler can find the mapper in this case.

Without this patch, it cannot recognize array with mapper, for instance, #pragma omp target map(mapper(a),to: arr[0:2]) won't work without this patch.

What if we have a mapper for the array type?

Without this patch, it won't be able to find the mapper in this case, and Clang assumes no mapper. This patch fixes it, so the compiler can find the mapper in this case.

It means, that you just ignore mappers for the array types, right?

Without this patch, it cannot recognize array with mapper, for instance, #pragma omp target map(mapper(a),to: arr[0:2]) won't work without this patch.

What if we have a mapper for the array type?

Without this patch, it won't be able to find the mapper in this case, and Clang assumes no mapper. This patch fixes it, so the compiler can find the mapper in this case.

It means, that you just ignore mappers for the array types, right?

Yes, it ignored mappers before, and this patch fixes it.

Definitely need positive tests with ast printing and codegen.

lib/Sema/SemaOpenMP.cpp
14751 ↗(On Diff #221577)

Why do you want canonical type here? I think it is wrong. It drops all language sugar like typedefs etc. But typedefs are not supported in mappers, right?

lildmh marked an inline comment as done.Sep 25 2019, 1:52 PM

HI Alexey, the ast print test is already there. Because I didn't check the mapper for array type before, such code will always not report any error, and ast print test is correct. Codegen test belongs to the other patch I released. It fits that patch much better.

lib/Sema/SemaOpenMP.cpp
14751 ↗(On Diff #221577)

I didn't see that the spec says typedef is not supported in mappers, so I suppose it should be supported. So I think getCanonicalType is necessary here?

HI Alexey, the ast print test is already there. Because I didn't check the mapper for array type before, such code will always not report any error, and ast print test is correct. Codegen test belongs to the other patch I released. It fits that patch much better.

How is this possible? If we did not have support for the array type, we could not have correct handling of such types in successful tests.

lib/Sema/SemaOpenMP.cpp
14751 ↗(On Diff #221577)

Do we check the canonicaL type by default, for non-array type?

lildmh marked an inline comment as done.Sep 26 2019, 6:11 AM

HI Alexey, the ast print test is already there. Because I didn't check the mapper for array type before, such code will always not report any error, and ast print test is correct. Codegen test belongs to the other patch I released. It fits that patch much better.

How is this possible? If we did not have support for the array type, we could not have correct handling of such types in successful tests.

The ast print for array with mapper was correct because the mapper id is still with the array type. Without this patch, the problem is it will not look up the mapper declaration associated with the id, and as a result, the codegen is not correct. I found this problem when I tested the codegen.

lib/Sema/SemaOpenMP.cpp
14751 ↗(On Diff #221577)

Yes, we use canonical type for non-array type, for instance, line 14935 in this file.

HI Alexey, the ast print test is already there. Because I didn't check the mapper for array type before, such code will always not report any error, and ast print test is correct. Codegen test belongs to the other patch I released. It fits that patch much better.

How is this possible? If we did not have support for the array type, we could not have correct handling of such types in successful tests.

The ast print for array with mapper was correct because the mapper id is still with the array type. Without this patch, the problem is it will not look up the mapper declaration associated with the id, and as a result, the codegen is not correct. I found this problem when I tested the codegen.

Then another one question. Why we don't emit the diagnostics if the original type is not a class, struct or union? We just ignore this situation currently but we should not. The error message must be emitted. I think that's why the ast-print test works correctly though it should not.

HI Alexey, the ast print test is already there. Because I didn't check the mapper for array type before, such code will always not report any error, and ast print test is correct. Codegen test belongs to the other patch I released. It fits that patch much better.

How is this possible? If we did not have support for the array type, we could not have correct handling of such types in successful tests.

The ast print for array with mapper was correct because the mapper id is still with the array type. Without this patch, the problem is it will not look up the mapper declaration associated with the id, and as a result, the codegen is not correct. I found this problem when I tested the codegen.

Then another one question. Why we don't emit the diagnostics if the original type is not a class, struct or union? We just ignore this situation currently but we should not. The error message must be emitted. I think that's why the ast-print test works correctly though it should not.

We emit such diagnostic when mapper is declared, i.e., you cannot define a mapper for a non-class, non-struct, non-union type. It's in function ActOnOpenMPDeclareMapperType in SemaOpenMP.cpp. But I didn't emit such information where mapper is used. I will fix it in this patch. Thanks for getting this!

lildmh updated this revision to Diff 221990.Sep 26 2019, 11:14 AM
lildmh updated this revision to Diff 221991.
lildmh retitled this revision from [OpenMP 5.0] Fix user-defined mapper lookup in sema for arrays to [OpenMP 5.0] Fix user-defined mapper lookup in sema.
lildmh edited the summary of this revision. (Show Details)

Fix mapper type checking

ABataev added inline comments.Sep 26 2019, 11:21 AM
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

Why need an additional check for scope and not "default" id? I don't see this additional requirement in the standard.

lildmh marked an inline comment as done.Sep 26 2019, 11:29 AM
lildmh added inline comments.
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

It's because every variable in map clauses will check this, including those are not struct, class, or union.

Using this, e.g., mapping an integer won't report error that it doesn't have a mapper.

ABataev added inline comments.Sep 26 2019, 11:34 AM
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

Maybe just move the check to the end of the function?

lildmh marked an inline comment as done.Sep 26 2019, 11:54 AM
lildmh added inline comments.
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

It will do some additional useless work if I move it to the end. I don't think it is necessary to move it back.

But if you believe it's better, I can do it.

ABataev added inline comments.Sep 26 2019, 12:45 PM
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

Could you add tests for this new functionality?

lildmh marked an inline comment as done.Sep 26 2019, 1:13 PM
lildmh added inline comments.
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

I already add the test in declare_mapper_message.c. I think nothing is needed for ast print test.

ABataev added inline comments.Sep 26 2019, 1:23 PM
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

I mean, for this error message. I did not see the change in the tests when you added this new check.

lildmh marked an inline comment as done.Sep 26 2019, 1:28 PM
lildmh added inline comments.
lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

line 55, 68, 79 in declare_mapper_messages.c is added for this new check.

ABataev accepted this revision.Sep 26 2019, 1:33 PM

LG.

lib/Sema/SemaOpenMP.cpp
14805 ↗(On Diff #221991)

Ah, ok, found it, thanks!

This revision is now accepted and ready to land.Sep 26 2019, 1:33 PM

Thanks Alexey! Please check the other 2 mapper patches at https://reviews.llvm.org/D67833 and https://reviews.llvm.org/D68100 when you have time. They should be the last mapper patches.

Thanks Alexey! Please check the other 2 mapper patches at https://reviews.llvm.org/D67833 and https://reviews.llvm.org/D68100 when you have time. They should be the last mapper patches.

Sure, will do this tomorrow.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 3:51 PM