amitamd7 wrote: > I have revisited this PR (I am sorry I got distracted and forgot to merge it). > > I confirmed the issues still persist in the latest version and that this > fixes them. > > In the latest version of the PR I also added the test above as a test under > `offload`. However, while I was testing, I found that some tests have been > introduced since I submitted the PR which in my opinion are wrong, namely > these two: > > ``` > offload/test/offloading/strided_update_variable_stride_misc.c > offload/test/offloading/strided_update_count_expression_complex.c > ``` > > I think both were introduced here #181987 > > @amitamd7 Would you be able to confirm that I am not missing something here? > > For the first test: > > ``` > offload/test/offloading/strided_update_variable_stride_misc.c > ``` > > From my reading of it, the result for `// CHECK: Test 1: Variable stride = 1` > should all be updated because we update with slice [0:10:1], however the > CHECK lines represent the state before any update. I fixed this to have all > indices updated in the CHECK lines and with this patch it seems to pass. > > For the second test: > > ``` > offload/test/offloading/strided_update_count_expression_complex.c > ``` > > The checklines for this specific case seem wrong: > > ``` > // CHECK: Test 2 - complex count with offset (from): > // CHECK: s1 results: > ``` > > We update the array with the slice [2:4:2] but in the original CHECK lines, > the update started at 3rd index instead of the 2nd. So I have fixed the CHECK > lines to represent the result I would expect and marked the test as XFAIL > because it fails. The reason it fails seems unrelated to what this patch is > attempting to fix, and I think it is related to the update command getting > the address of the struct s1 as a whole instead of the array inside it for > the base pointer of the update operation. > > (cc'ing the reviewers of the other patch for visibility @alexey-bataev > @abhinavgaba)
For test1 (strided_update_variable_stride_misc.c): You're correct. The CHECK lines I wrote reflect the original host state before the update -- that was my mistake. The slice [0:10:1] with stride=1 transfers all 10 elements from the device, so the expected output should be something like [0, 2, 4, 6, 8,...] (each element 2*i after data1[i] += i). For test2 (strided_update_count_expression_complex.c) You're correct here as well. s1.arr[2 : 4 : 2] should update indices 2, 4, 6,... but my CHECK lines show updates at indices 3, 5, 7, 9 which is off by one from the starting offset. Your corrected CHECK lines and the XFAIL marking for the separate base-pointer issue make sense to me. Thank you for fixing these in your patch, and catching the mistakes. Please go ahead with this one. https://github.com/llvm/llvm-project/pull/156889 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
