[mpich-discuss] initialization issue in strided_get_indexed

[sreeram potluri]

Thanks much for the fix Jim!!

Sreeram Potluri

On Mon, Dec 12, 2011 at 6:30 PM, Jim Dinan <dinan at mcs.anl.gov> wrote:

> Hi Sreeram,
>
> I committed a fix to the MPICH2 trunk (r9335).  I added the missing
> initializations and set local buffers to -1.0 to make the tests more robust
> when run on a single process.  I also fixed those bad floating point
> comparisons.
>
> Thanks again for reporting this.
>
> Best wishes,
>  ~Jim.
>
>
> On 12/12/11 4:16 PM, Jim Dinan wrote:
>
>> Hi Sreeram,
>>
>> Your understanding of the test case is spot on; I just didn't read it
>> closely enough before responding. I think this bug is the culprit:
>>
>> if (actual - expected > 1e-10) {
>>
>> This should have an fabs(), somehow we've just been getting lucky:
>>
>> if (fabs(actual - expected) > 1e-10) {
>>
>> This affects all of those strided tests. I'll put together a fix and let
>> you know as soon as it's checked in. Great catch and thanks for the bug
>> report!
>>
>> Best,
>> ~Jim.
>>
>>
>> On 12/12/11 3:47 PM, sreeram potluri wrote:
>>
>>> Jim,
>>>
>>> My understanding of the test is that only a part of the loc_buf is
>>> overwritten by the MPI_Get operation (8*256 rather than 8*1024).
>>>
>>> Now the test also tries to validate the rest of the loc_buf which is not
>>> affected by the MPI_Get operation. This check can fail or pass based on
>>> the original value of the loc_buf, which is never set.
>>>
>>> Am I missing something basic?
>>>
>>> Sreeram Potluri
>>>
>>> On Mon, Dec 12, 2011 at 3:11 PM, Jim Dinan <dinan at mcs.anl.gov
>>> <mailto:dinan at mcs.anl.gov>> wrote:
>>>
>>> Hi Sreeram,
>>>
>>> Setting the initial value of loc_buf shouldn't influence whether or
>>> not this test passes; the entire contents of loc_buf should be
>>> overwritten later by the call to MPI_Get. As Dave pointed out,
>>> setting this to 1.0+rank also weakens the test when run on one
>>> process since loc_buf will already contain the correct data. This
>>> is not the case when run on more processes since loc_buf should
>>> contain 1.0+((rank+1)%nproc)).
>>>
>>> This test has been in the MPICH test suite for several months and
>>> hasn't given us any trouble. Any chance that it's exercising a bug
>>> on your end?
>>>
>>> Best,
>>> ~Jim.
>>>
>>>
>>> On 12/12/11 1:27 PM, Dave Goodell wrote:
>>>
>>> Sreeram,
>>>
>>> I'm not very familiar with this test, but this looks like an
>>> incorrect change to me. By setting the loc_buf (the origin
>>> "receive" buffer) to the expected value in this way then you
>>> open up a testing coverage gap if MPI_Get is replaced with a
>>> no-op function.
>>>
>>> If anything, this test could probably be strengthened by adding
>>> the following line in the same location that you recommend
>>> making your change:
>>>
>>> ----8<----
>>> *(loc_buf + i) = -1.0;
>>> ----8<----
>>>
>>> This way you don't assume anything about the values already in
>>> the buffer returned by MPI_Alloc_mem.
>>>
>>> Perhaps Jim can comment, since he wrote the test originally.
>>>
>>> -Dave
>>>
>>> On Dec 12, 2011, at 6:53 AM CST, sreeram potluri wrote:
>>>
>>> Hi Team,
>>>
>>> We have seen a validation error with the strided_get_indexed
>>> test in our internal testing with MVAPICH2. This appears to
>>> be an initialization issue with the benchmark. The path
>>> below fixes it in one way. Please accept the change if
>>> appropriate.
>>>
>>> Index: mpi/rma/strided_get_indexed.c
>>> ==============================**__============================**
>>> ==__=======
>>> --- mpi/rma/strided_get_indexed.c (revision 5064)
>>> +++ mpi/rma/strided_get_indexed.c (working copy)
>>> @@ -48,8 +48,10 @@
>>> if (rank == 0)
>>> if (verbose) printf("MPI RMA Strided Get Test:\n");
>>>
>>> - for (i = 0; i< XDIM*YDIM; i++)
>>> + for (i = 0; i< XDIM*YDIM; i++) {
>>> *(win_buf + i) = 1.0 + rank;
>>> + *(loc_buf + i) = 1.0 + rank;
>>> + }
>>>
>>> MPI_Win_create(win_buf, bufsize, 1, MPI_INFO_NULL,
>>> MPI_COMM_WORLD,&buf_win);
>>>
>>> Regards
>>> Sreeram Potluri
>>> ______________________________**___________________
>>> mpich-discuss mailing list mpich-discuss at mcs.anl.gov
>>> <mailto:mpich-discuss at mcs.anl.**gov <mpich-discuss at mcs.anl.gov>>
>>> To manage subscription options or unsubscribe:
>>> https://lists.mcs.anl.gov/__**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/__mailman/listinfo/mpich-discuss>
>>> <https://lists.mcs.anl.gov/**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/mailman/listinfo/mpich-discuss>
>>> **>
>>>
>>>
>>> ______________________________**___________________
>>> mpich-discuss mailing list mpich-discuss at mcs.anl.gov
>>> <mailto:mpich-discuss at mcs.anl.**gov <mpich-discuss at mcs.anl.gov>>
>>> To manage subscription options or unsubscribe:
>>> https://lists.mcs.anl.gov/__**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/__mailman/listinfo/mpich-discuss>
>>> <https://lists.mcs.anl.gov/**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/mailman/listinfo/mpich-discuss>
>>> **>
>>>
>>>
>>>
>>>
>>> ______________________________**_________________
>>> mpich-discuss mailing list mpich-discuss at mcs.anl.gov
>>> To manage subscription options or unsubscribe:
>>> https://lists.mcs.anl.gov/**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/mailman/listinfo/mpich-discuss>
>>>
>> ______________________________**_________________
>> mpich-discuss mailing list mpich-discuss at mcs.anl.gov
>> To manage subscription options or unsubscribe:
>> https://lists.mcs.anl.gov/**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/mailman/listinfo/mpich-discuss>
>>
> ______________________________**_________________
> mpich-discuss mailing list     mpich-discuss at mcs.anl.gov
> To manage subscription options or unsubscribe:
> https://lists.mcs.anl.gov/**mailman/listinfo/mpich-discuss<https://lists.mcs.anl.gov/mailman/listinfo/mpich-discuss>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/mpich-discuss/attachments/20111212/8e437063/attachment-0001.htm>