This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor class.
ClosedPublic

Authored by cchen on Feb 21 2020, 8:33 AM.

Details

Summary

This step is the preparation of allowing lvalue in map/motion clause.

Event Timeline

cchen created this revision.Feb 21 2020, 8:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
cchen marked an inline comment as done.Feb 21 2020, 8:40 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15255

@ABataev, in the previous patch you mentioned that we don't need IgnoreParenImpCasts() here, but I think I was just trying to do what line 15232 did.

ABataev added inline comments.Feb 21 2020, 9:27 AM
clang/lib/Sema/SemaOpenMP.cpp
15246

emitErrorMsg() here?

15247

No need cast here.

15248

Better to use IgnoreParenCasts() to handle explicit casting to the base class.

15255

Just Visit(E)

15260

Default initializer here

15262

Default initializer

15263–15265

Better to use = <init>; here. Just a nit.

15278

Just Visit(E)

15283

Just Visit(E)

15300

Just Visit(E)

15319

Visit(E)

15336–15337

E->IgnoreParensCasts()

15338

Need to check that AE->getIdx() is not value dependent, otherwise it may crash

15369–15370

Just Visit(E)

15402–15411

Also, need to be sure that the expressions here are not value-dependent.

15445–15474

Just add:

boo VisitStmt(Stmt *) {
  emitErrorMsg();
  return false;
}

instead of all these functions, returning false as a result.

15475–15477

No need for else here, just unconditional return nullptr.

cchen marked an inline comment as done.Feb 21 2020, 10:00 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15338

It seems Clang would catch the error before we do the analysis:

orig.cpp:6:24: error: array subscript is not an integer
#pragma omp target map(a[b])
                       ^ ~
orig.cpp:15:3: note: in instantiation of function template specialization 'gg<int, double>' requested here
  gg<int, double>(a, c);
  ^
orig.cpp:8:5: error: array subscript is not an integer
    a[b] = 10;
    ^ ~
2 errors generated.

Also, if we still need it, do we also check type dependent?

ABataev added inline comments.Feb 21 2020, 10:09 AM
clang/lib/Sema/SemaOpenMP.cpp
15338
  1. Yes, it will find incorrect expression at the instantiation. But what if you're working with the template? In this case, the expression can be value-dependent.
  2. No, no need to check for type-dependence here, a check for value-dependent expression should be enough.
cchen updated this revision to Diff 245912.Feb 21 2020, 10:17 AM

Fix based on feedback.

cchen marked 17 inline comments as done.Feb 21 2020, 10:19 AM
cchen retitled this revision from [OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor clause. to [OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor class..
cchen added a project: Restricted Project.
ABataev added inline comments.Feb 21 2020, 10:58 AM
clang/lib/Sema/SemaOpenMP.cpp
15319

return RelevantExpr || Visit(E);?

15346

return RelevantExpr || Visit(E);?

15357–15358
  1. Need to check AE->getIdx(), not AE.
  2. Why return false here? I would say, just skip the check.
15372–15373

return RelevantExpr || Visit(E);?

15418–15427

Need a check for value-dependent OASE->getLowerBound()

15420–15421
  1. Need to check OASE->getLength(), not OASE.
  2. Why return false here? I would say, just skip the check.
15443

Just Visit(E);

15452

Do you really need this function?

15461–15477

Use \\\ style of comment here

cchen updated this revision to Diff 245955.Feb 21 2020, 12:08 PM

Fix by feedback

cchen marked 11 inline comments as done.Feb 21 2020, 12:09 PM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15452

Removed the function.

Did you try to run the tests? I would suggest adding a test, at least, where a function is mapped. We should see the error message here. Seems to me, we don't have this one.

clang/lib/Sema/SemaOpenMP.cpp
15400–15403

Same here, try to reduce complexity

15402–15403

Just a nit. Try to reduce the structural complexity here by using logical exressions, if (!OASE->getLength()->isValueDependent() && OASE->getLength()->EvaluateAsInt(...) && !ResultR.Val.getInt().isOneValue()). Also, you can

cchen marked an inline comment as done.Feb 21 2020, 12:54 PM

Did you try to run the tests? I would suggest adding a test, at least, where a function is mapped. We should see the error message here. Seems to me, we don't have this one.

We already have test for err_omp_invalid_map_this_expr, note_omp_invalid_subscript_on_this_ptr_map, etc.. in target_message.cpp line 44 without checking for value dependence. Do you mean that I should add a test for the check of value dependence? In that case, we don't print any messages.

ABataev added a comment.EditedFeb 21 2020, 12:59 PM

Did you try to run the tests? I would suggest adding a test, at least, where a function is mapped. We should see the error message here. Seems to me, we don't have this one.

We already have test for err_omp_invalid_map_this_expr, note_omp_invalid_subscript_on_this_ptr_map, etc.. in target_message.cpp line 44 without checking for value dependence. Do you mean that I should add a test for the check of value dependence? In that case, we don't print any messages.

No. But previously, if we tried to map a function, not a variable, it would be skipped silently. With this patch, this behavior will change. I suggest adding a test with mapping a function to check that error message is emitted. Something like:

int foo();
#pragma omp target map(foo) // <- must be an error here

And I just asked if you tried to run the tests with this patch. Did the result change or it is the same?

cchen added a comment.Feb 21 2020, 1:15 PM

Did you try to run the tests? I would suggest adding a test, at least, where a function is mapped. We should see the error message here. Seems to me, we don't have this one.

We already have test for err_omp_invalid_map_this_expr, note_omp_invalid_subscript_on_this_ptr_map, etc.. in target_message.cpp line 44 without checking for value dependence. Do you mean that I should add a test for the check of value dependence? In that case, we don't print any messages.

No. But previously, if we tried to map a function, not a variable, it would be skipped silently. With this patch, this behavior will change. I suggest adding a test with mapping a function to check that error message is emitted. Something like:

int foo();
#pragma omp target map(foo) // <- must be an error here

And I just asked if you tried to run the tests with this patch. Did the result change or it is the same?

Got it, I'll add test for function mapping. I've run the test and this patch passed all the test.

cchen updated this revision to Diff 245978.Feb 21 2020, 1:23 PM

Add test for mapping function

ABataev added inline comments.Feb 21 2020, 1:30 PM
clang/lib/Sema/SemaOpenMP.cpp
15357–15359

Simplify this nested ifs too, please.

clang/test/OpenMP/target_messages.cpp
53–57

You mapped CallExprs here, not DeclRefs

119–125

same here, mapped CallExprs

cchen updated this revision to Diff 245983.Feb 21 2020, 1:45 PM

Fix test and sorry for my carelessness

Fix test and sorry for my carelessness

No problem. But I don't see the changes in the test.

clang/lib/Sema/SemaOpenMP.cpp
15473

Remove braces here, the substatement is single line.

cchen updated this revision to Diff 245995.Feb 21 2020, 2:20 PM

Update test

ABataev accepted this revision.Feb 21 2020, 2:27 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Feb 21 2020, 2:27 PM
cchen added a comment.Feb 21 2020, 2:35 PM

@ABataev, can you land it for me when you have time? Thanks

This revision was automatically updated to reflect the committed changes.
estewart08 added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
15452

Was this function intended to be removed? As far as I can tell it was not and it seems to be the source of an issue I am having:

expected addressable lvalue in 'map' clause

Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 1:29 PM