[SYCL] Changes to visitor model in preparation of array support. #1764

rdeodhar · 2020-05-27T20:03:57Z

Signed-off-by: rdeodhar [email protected]

Signed-off-by: rdeodhar <[email protected]>

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-05-27T20:08:46Z

clang/lib/Sema/SemaSYCL.cpp

+template <typename RangeTy, typename... Handlers>
+static void VisitField(CXXRecordDecl *Owner, RangeTy Item, QualType ItemTy,
+                       Handlers &... handlers) {
+  if (Util::isSyclAccessorType(ItemTy)) {


Don't use curley brackets for single line if-statements

erichkeane · 2020-05-27T20:09:35Z

clang/lib/Sema/SemaSYCL.cpp

+  } else if (Util::isSyclStreamType(ItemTy))
+    (void)std::initializer_list<int>{
+        (handlers.handleSyclStreamType(Item, ItemTy), 0)...};
+  else {


This else body shouldn't be necessary, make the two checks top-level. Additionally, please leave the existing order (where structure/class type happens right after accessor/stream) and put the array at the end.

erichkeane · 2020-05-27T20:11:07Z

clang/lib/Sema/SemaSYCL.cpp

+  int64_t ElemCount = CAT->getSize().getSExtValue();
+  std::initializer_list<int>{(handlers.enterArray(), 0)...};
+  for (int64_t Count = 0; Count < ElemCount; Count++) {
+    VisitField(nullptr, Item, ET, handlers...);


It seems that 'visitField' could instead have its 'owner' be a generic declaration, so that the array type could pass that on instead. This would also allow offset-handling quite a bit more easily I think.

I'm not sure about this. Let's wait until the handleArrayType bodies are filled in.

erichkeane · 2020-05-27T20:14:19Z

clang/lib/Sema/SemaSYCL.cpp

+  bool copyableToKernel(const FieldDecl *FD, const QualType &FieldTy) {
+    // C++ lambda capture already flags non-constant array types.
+    // Here, we check copyability.
+    if (FieldTy->isConstantArrayType()) {


Suggested change

if (FieldTy->isConstantArrayType()) {

if (const auto *CAT = dyn_cast<ConstantArrayType>(FieldTy)) {

And so fourth, do the same elsewhere if you're going to use the type.

erichkeane · 2020-05-27T20:15:28Z

clang/lib/Sema/SemaSYCL.cpp

@@ -801,6 +843,43 @@ class SyclKernelFieldChecker
  bool IsInvalid = false;
  DiagnosticsEngine &Diag;

+  // Check whether the object is bit-wise copyable
+  bool copyableToKernel(const FieldDecl *FD, const QualType &FieldTy) {


This is actually a 'check' type function, and thus should be 'checkCopyableToKernel'. With that, please invert the 'bool' (false is successful). Then, all of the Diagnostics.report, followed by a return changes to "return Diagnostics.Report", since that is implicitly convertable to bool.

erichkeane · 2020-05-27T20:15:46Z

clang/lib/Sema/SemaSYCL.cpp

+        return false;
+      }
+    }
+    if (!FieldTy->isStructureOrClassType()) {


no curleys for single liners.

erichkeane · 2020-05-27T20:16:15Z

clang/lib/Sema/SemaSYCL.cpp

+  }
+
+  void handleArrayType(const CXXBaseSpecifier &, QualType) final {
+    // FIXME


Add some text here, what do you intend this fixme to do?

Fznamznon · 2020-05-28T16:34:41Z

clang/lib/Sema/SemaSYCL.cpp

+template <typename RangeTy, typename... Handlers>
+static void VisitArrayElements(RangeTy Item, QualType FieldTy,
+                               Handlers &... handlers) {
+  const ConstantArrayType *CAT = cast<ConstantArrayType>(FieldTy);


When you do cast to ConstantArrayType you expect that FieldTy is ConstantArrayType, otherwise assertion will be triggered, so, is it safe to do cast here? I'm asking because VisitArrayElements is called after check on isArrayType, not on isConstantArrayType.

Later, he leaves this comment, which made me not comment on this. but it is still worth evaluating:
// C++ lambda capture already flags non-constant array types.
// Here, we check copyability.

There are 4 types of arrays:
Constant
This one is obvious.

DependentSized
I dont think we have to worry about this one, all instantiations are done before we get here, right?

Incomplete
You cannot actually capture an incomplete sized array type as far as I can tell. They aren't particularly easy to spell in C, and I think we prevent capturing them: https://godbolt.org/z/-oDsdz

Variable
This is the only one that I think is possible, but only as a reference type: https://godbolt.org/z/hEdiK-
Of course, VLAs in structs aren't supported in clang (with the pretty assertive message: fields must have a constant size: 'variable length array in structure' extension will never be supported)

That said, Do we prevent the reference case from making it here?

AFAIK SYCL spec allows only "by value" ([=] this one) capture for kernels, AFAIK we just don't diagnose if someone tried to use "by reference" capture.
This comment didn't satisfy me because kernel object can be defined not only as lambda but as user-defined callable object as well.

Well, then the VariableArraytype isn't a concern, though we should probably diagnose that (should we diagnose ALL ReferenceType fields?).

I'd not thought about other object types. From that perspective, only the incomplete type is of concern, we should likely diagnose that.

FYI: This is another great use of the 'have visit methods return the boolean value'. We could/should stop descending once we report failure.

Oh, I just forgot, but it seems we already diagnose ReferenceType fields, don't we?

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 810 in 08f8656

IsInvalid = Diag.Report(FD->getLocation(), diag::err_bad_kernel_param_type)

From that perspective, only the incomplete type is of concern, we should likely diagnose that.

Agree.

Non-constant arrays could slip through via function objects. I've added a check.

erichkeane

I know you emailed a separate implementation that you're working on, so I'll hold off on further review. That said, see my comment here:

We'd like this return value to propogate and happen everywhere, it doesn't make sense to continue looking into any handlers if the 'base' had failed, even struct descent.

erichkeane · 2020-06-01T13:09:20Z

clang/lib/Sema/SemaSYCL.cpp

+}
+template <typename T, typename... Tn>
+static bool handleField(FieldDecl *FD, QualType FDTy, T &t, Tn &... tn) {
+  return (t.first->*t.second)(FD, FDTy) && handleField(FD, FDTy, tn...);


I think this "&&" logic needs to happen at every level here. All the way to the root fields, base classes, etc. As soon as we discover an error in attempting anything we need to give up. So the 'bool' needs to be propogated quite a bit more.

I would think it should cause us to stop descending as well.

Fznamznon · 2020-06-04T14:02:30Z

clang/lib/Sema/SemaSYCL.cpp

+    bool bad = Diag.Report(FD->getLocation(), diag::err_bad_kernel_param_type)
+               << FieldTy;
+    IsInvalid |= bad;
+    return !bad;


AFAIK bad here is always true. @erichkeane , should we return true in handler functions to continue?

bad is completely unnecessary here, I think 934 should just be: "IsInvalid = true", and return false.

You're right, Diag.Report ALWAYS returns true. I'd also suggest having each of these functions just return isValid().

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-06-04T15:56:28Z

clang/lib/Sema/SemaSYCL.cpp

+  std::cerr << "isSyclAccessorType:Ty\n";
+  Ty->dump();
+  bool b = isSyclType(Ty, "accessor", true /*Tmpl*/) || isSyclType(Ty, "accessor_common", true /*Tmpl*/);
+  std::cerr << (b ? "IS"
+                 : "NOT")
+                       << " an accessor\n\n";
+  return b;


Oh, it seems something wrong happened, right?

erichkeane

Looks fine to me too. However, @bader, it seems to fail in the merge step. Whats the best way to do this? SHould he commit a merge commit, or do a rebase/force-push, or what?

bader · 2020-06-05T12:51:22Z

To avoid force-pushes it's re-commended to resolve conflict using git merge.

rdeodhar · 2020-06-05T17:42:19Z

I've merged in the latest intel/llvm. It should be mergeable now.

bader

LGTM.
One minor style comment - please, apply in follow-up PR(s).

bader · 2020-06-06T16:34:24Z

clang/lib/Sema/SemaSYCL.cpp

+      if (const auto *CAT = dyn_cast<ConstantArrayType>(FieldTy)) {
+        QualType ET = CAT->getElementType();
+        return checkNotCopyableToKernel(FD, ET);
+      } else


Suggested change

} else

}

Please, don't use else after return.

[SYCL] Changes to visitor model in preparation of array support.

95ea801

Signed-off-by: rdeodhar <[email protected]>

rdeodhar requested review from erichkeane and Fznamznon as code owners May 27, 2020 20:03

erichkeane reviewed May 27, 2020

View reviewed changes

Fznamznon reviewed May 28, 2020

View reviewed changes

Added conditional execution of handlers.

c09dae4

erichkeane reviewed Jun 1, 2020

View reviewed changes

Changes to visitor model to run handlers conditionally.

f1161d6

rdeodhar requested review from erichkeane and Fznamznon June 3, 2020 17:29

Fznamznon reviewed Jun 4, 2020

View reviewed changes

Corrections to returning validity.

97ddadb

rdeodhar requested a review from Fznamznon June 4, 2020 15:55

Fznamznon reviewed Jun 4, 2020

View reviewed changes

Corrected file upload.

a16091a

Fznamznon previously approved these changes Jun 4, 2020

View reviewed changes

erichkeane previously approved these changes Jun 5, 2020

View reviewed changes

Merge branch 'sycl' of https://github.com/otcshare/llvm into akp1

2a36ef6

rdeodhar dismissed stale reviews from erichkeane and Fznamznon via 2a36ef6 June 5, 2020 17:30

erichkeane approved these changes Jun 5, 2020

View reviewed changes

bader approved these changes Jun 6, 2020

View reviewed changes

bader merged commit 2b72f8b into intel:sycl Jun 6, 2020

Fznamznon mentioned this pull request Jun 8, 2020

[WIP][Do not Review][SYCL] Decompose Kernel Parameters #1833

Closed

elizabethandrews mentioned this pull request Jun 10, 2020

Support for arrays as kernel parameters. #1841

Merged

rdeodhar deleted the akp1 branch January 15, 2021 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Changes to visitor model in preparation of array support. #1764

[SYCL] Changes to visitor model in preparation of array support. #1764

rdeodhar commented May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

erichkeane May 27, 2020

rdeodhar May 27, 2020

Fznamznon May 28, 2020

erichkeane May 28, 2020

Fznamznon May 28, 2020

erichkeane May 28, 2020

erichkeane May 28, 2020

Fznamznon May 28, 2020 •

edited

Loading

rdeodhar May 30, 2020

erichkeane left a comment

erichkeane Jun 1, 2020

Fznamznon Jun 4, 2020

erichkeane Jun 4, 2020

rdeodhar Jun 4, 2020

Fznamznon Jun 4, 2020

erichkeane left a comment

bader commented Jun 5, 2020

rdeodhar commented Jun 5, 2020 •

edited by bader

Loading

bader left a comment

bader Jun 6, 2020

	if (FieldTy->isConstantArrayType()) {
	if (const auto *CAT = dyn_cast<ConstantArrayType>(FieldTy)) {

[SYCL] Changes to visitor model in preparation of array support. #1764

[SYCL] Changes to visitor model in preparation of array support. #1764

Conversation

rdeodhar commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fznamznon May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

bader commented Jun 5, 2020

rdeodhar commented Jun 5, 2020 • edited by bader Loading

bader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fznamznon May 28, 2020 •

edited

Loading

rdeodhar commented Jun 5, 2020 •

edited by bader

Loading