[SYCL] Fix crash when kernel argument is a multi-dimensional array. #2341

elizabethandrews · 2020-08-19T02:39:37Z

This patch fixes crash due to incorrect InitializedEntity for
multi-dimensional arrays. When generating the InitializedEntity
for an element, it is necessary to descend the array. For example,
the initialized entity for s.array[x][y][z] is constructed using
initialized entities for s.array[x][y], s.array[x] and s.array.
Prior to this patch, the 'descending' was not done.

Patch by: Rajiv Deodhar and Elizabeth Andrews

Signed-off-by: Elizabeth Andrews [email protected]

This patch fixes crash due to incorrect InitializedEntity for multi-dimensional arrays. When generating the InitializedEntity for an element, it is necessary to descend the array. For example, the initialized entity for s.array[x][y][z] is constructed using initialized entities for s.array[x][y], s.array[x] and s.array. Prior to this patch, the 'descending' was not done. Patch by: Rajiv Deodhar and Elizabeth Andrews Signed-off-by: Elizabeth Andrews <[email protected]>

from MemberExprBases. Also fixes a mistake with offset calculation for integration headers generated. Adds lit tests. Patch by: Rajiv Deodhar and Elizabeth Andrews Signed-off-by: Elizabeth Andrews <[email protected]>

v-klochkov · 2020-08-20T06:06:46Z

/summary:run

Fznamznon

Looks very complicated and confusing.

clang/lib/Sema/SemaSYCL.cpp

erichkeane

I'll leave it to @Fznamznon to finish the review, but this is a move in the right direction.

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews

Thanks for the review everyone! I will upload a new patch with the review comments.

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews · 2020-08-20T13:46:13Z

Does anyone know why the tests aren't running? @bader

bader · 2020-08-20T13:51:26Z

I suppose CI is busy with pre-commits for other PRs. Did you check the Buildbot queue?

elizabethandrews · 2020-08-20T13:57:35Z

I suppose CI is busy with pre-commits for other PRs. Did you check the Buildbot queue?

How do I do that?

DoyleLi · 2020-08-20T14:01:12Z

I suppose CI is busy with pre-commits for other PRs. Did you check the Buildbot queue?

How do I do that?

Hi, I find the root cause for not launching CI checks is the github didn't send out webhook event for this commit in the PR. Should be github sever issue. It can be launched if someone approve the change or new commit pushed in the PR.

elizabethandrews · 2020-08-20T14:04:22Z

I suppose CI is busy with pre-commits for other PRs. Did you check the Buildbot queue?

How do I do that?

Hi, I find the root cause for not launching CI checks is the github didn't send out webhook event for this commit in the PR. Should be github sever issue. It can be launched if someone approve the change or new commit pushed in the PR.

Thanks for the information! I will be pushing a new commit. So hopefully that will trigger it.

Fznamznon · 2020-08-20T16:03:11Z

Please resolve conflicts.

Added a few more comments, renamed variables, etc Signed-off-by: Elizabeth Andrews <[email protected]>

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

v-klochkov

I don't see anything wrong in this patch.
Couple places still remain not clear to me, thus I rely on review/approvals from FE experts @erichkeane and @Fznamznon

clang/lib/Sema/SemaSYCL.cpp

clang/test/SemaSYCL/array-kernel-param.cpp

Fznamznon

Just a couple of code-style nits. Otherwise LGTM

clang/lib/Sema/SemaSYCL.cpp

Renamed a variable Simplified MemberExprBasesIdx calculation removing +1/-1 Changed type of Index to uint64_t Signed-off-by: Elizabeth Andrews <[email protected]>

keryell

It is too easy to get wrong because of symmetry mirages...

keryell · 2020-08-21T08:27:52Z

clang/lib/Sema/SemaSYCL.cpp

+    // Each dimension has an ArraySubscriptExpression (maintains index)
+    // in MemberExprBases. For example, if we are currently handling element
+    // a[0][0][1], the top of stack entries are ArraySubscriptExpressions for
+    // indices 0,0 and 1, with 1 on top.


Do not use same numbers, it is too confusing.
We had this issue in the spec...

keryell · 2020-08-21T08:29:39Z

clang/lib/Sema/SemaSYCL.cpp

+    // We need to find the type of the element for which we are generating the
+    // InitListExpr. For example, for a multi-dimensional array say a[2][3][2],
+    // the types for InitListExpr of the array and its 'sub-arrays' are -
+    // int [2][3][2], int [3][2] and int [2]. This loop is used to obtain this


Do not use same numbers, it is too confusing.
I found using prime number is even better in the case of concrete computations.
For example here int [2] is not instantaneously obvious which [2] we are talking about.
Use for example int [5][3][2]

keryell · 2020-08-21T08:32:21Z

clang/test/CodeGenSYCL/kernel-param-pod-array-ih.cpp

@@ -46,9 +74,21 @@ __attribute__((sycl_kernel)) void a_kernel(const Func &kernelFunc) {
 int main() {

  int a[5];
+  int b[2][3];
+  int c[2][3][2];


Suggested change

int c[2][3][2];

int c[2][3][5];

keryell · 2020-08-21T08:32:31Z

clang/test/CodeGenSYCL/kernel-param-pod-array-ih.cpp

+
+  a_kernel<class kernel_D>(
+      [=]() {
+        int local = c[0][1][1];


Suggested change

int local = c[0][1][1];

int local = c[0][1][2];

bader · 2020-08-21T09:13:57Z

@elizabethandrews, please, address comments from @keryell in a separate PR. These comments do not seem to be blocking, so I'll merge.

OpenCL and NonSemantic DebugInfo specifications are flexible in terms of allowing any debug information be replaced with DebugInfoNone, so various of SPIR-V producers follow that and generate it for base types of several debug instructions, leaving SPIR-V consumers to handle this. By default the translator replaces missing debug info with tag: null, which is in most cases correct. Yet, there are situations, where it's not allowed by both LLVM and DWARF, for example for DW_TAG_array_type DWARF spec sets, that DW_AT_type attribute is mandatory. For such cases new transNonNullDebugType wrapper function was added to the translator, generating "DIBasicType(tag: DW_TAG_unspecified_type, name: "SPIRV unknown type")" where DebugInfoNone was used as the type. This function doesn't replace all calls to transDebugInst<DIType> as there are cases, where we can generate null type, for example DWARF doesn't require it for DW_TAG_typedef, hence I'm not changing translation flow in this case. Additionally to this, while DWARF requires type attribute for DW_TAG_pointer_type, LLVM does not, hence I'm not changing translation flow in this case as well. Signed-off-by: Sidorov, Dmitry <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@ec023805a0ce26f

elizabethandrews requested review from Fznamznon and premanandrao as code owners August 19, 2020 02:39

elizabethandrews requested review from rdeodhar and erichkeane August 19, 2020 02:41

elizabethandrews changed the title ~~[SYCL] Fix crash when kernel argument is a multi-dimensional array.~~ [Do not commit][SYCL] Fix crash when kernel argument is a multi-dimensional array. Aug 19, 2020

Fixes ArrayIndex calculation. This information is now extracted

bb48b0f

from MemberExprBases. Also fixes a mistake with offset calculation for integration headers generated. Adds lit tests. Patch by: Rajiv Deodhar and Elizabeth Andrews Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews requested a review from a team as a code owner August 20, 2020 04:19

elizabethandrews requested a review from v-klochkov August 20, 2020 04:19

elizabethandrews changed the title ~~[Do not commit][SYCL] Fix crash when kernel argument is a multi-dimensional array.~~ [SYCL] Fix crash when kernel argument is a multi-dimensional array. Aug 20, 2020

Fznamznon reviewed Aug 20, 2020

View reviewed changes

erichkeane reviewed Aug 20, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

elizabethandrews commented Aug 20, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

elizabethandrews added 2 commits August 20, 2020 09:35

Implemented review comments:

7016d65

Added a few more comments, renamed variables, etc Signed-off-by: Elizabeth Andrews <[email protected]>

Merge remote-tracking branch 'intel_llvm/sycl' into multidim2

bc049e4

erichkeane previously approved these changes Aug 20, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

Clang-format fix

e7f0187

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed erichkeane’s stale review via e7f0187 August 20, 2020 17:43

erichkeane previously approved these changes Aug 20, 2020

View reviewed changes

v-klochkov previously approved these changes Aug 20, 2020

View reviewed changes

elizabethandrews requested a review from Fznamznon August 20, 2020 19:10

premanandrao reviewed Aug 20, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

clang/test/SemaSYCL/array-kernel-param.cpp Show resolved Hide resolved

Fznamznon previously approved these changes Aug 20, 2020

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

premanandrao previously approved these changes Aug 20, 2020

View reviewed changes

Apply review comments:

57d57d6

Renamed a variable Simplified MemberExprBasesIdx calculation removing +1/-1 Changed type of Index to uint64_t Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed stale reviews from premanandrao, Fznamznon, v-klochkov, and erichkeane via 57d57d6 August 20, 2020 20:03

elizabethandrews requested review from premanandrao, v-klochkov and Fznamznon August 20, 2020 20:35

premanandrao approved these changes Aug 21, 2020

View reviewed changes

erichkeane approved these changes Aug 21, 2020

View reviewed changes

v-klochkov approved these changes Aug 21, 2020

View reviewed changes

Fznamznon approved these changes Aug 21, 2020

View reviewed changes

keryell reviewed Aug 21, 2020

View reviewed changes

bader merged commit 36f6ab6 into intel:sycl Aug 21, 2020

[SYCL] Fix crash when kernel argument is a multi-dimensional array. #2341

[SYCL] Fix crash when kernel argument is a multi-dimensional array. #2341

Uh oh!

Conversation

elizabethandrews commented Aug 19, 2020

Uh oh!

v-klochkov commented Aug 20, 2020

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Aug 20, 2020

Uh oh!

bader commented Aug 20, 2020

Uh oh!

elizabethandrews commented Aug 20, 2020

Uh oh!

DoyleLi commented Aug 20, 2020

Uh oh!

elizabethandrews commented Aug 20, 2020

Uh oh!

Fznamznon commented Aug 20, 2020

Uh oh!

Uh oh!

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

keryell Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

bader commented Aug 21, 2020

Uh oh!

Uh oh!