Opened 5 years ago

Closed 3 years ago

#1837 closed bug (wontfix)

Progress model changes from IBM.

Reported by: blocksom Owned by: balaji
Priority: major Milestone: future
Component: mpich Keywords: ibm-integ
Cc:

Description

There are misc progress model changes that were not contributed to the mpich master branch. As there is no individual commit that contained these changes, these are likely leftovers from a goofed up merge.

Regardless, this code exists in the internal ibm repository and is not in the mpich repository. I would like to discuss these changes and perhaps find a solution that will allow this (or similar) code to be added to the mpich master branch so that it would no longer be necessary to maintain this code separately.

Change History (13)

comment:1 Changed 5 years ago by blocksom

See branch mpich-ibm/ticket-1837.

comment:2 Changed 5 years ago by balaji

  • Milestone set to mpich-3.1
  • Owner set to balaji

comment:3 Changed 5 years ago by balaji

There are several problems with this patch.

  1. There are a lot of C99-isms in the code. Fixed width integers, statements interleaved with declarations, declare-time structure field initializations, etc.
  1. There are a bunch of white-space changes that need to be removed. They are quite distracting.
  1. You are protecting all of the error checks within HAVE_ERROR_CHECKING. That's fine, except you are getting rid of some of the checks, such as the one for MPROBE. Is that just a merge error? Once this is fixed, that part can probably be a separate commit since it's independent of the rest.
  1. Are MPID_cc_prefetch and MPID_cc_prefetch_is_complete defined somewhere? Are you recommending adding them to the ADI?
  1. There's a weird two-level unrolling of the first for loop. What's the reason for this? Is the compiler not able to do this unrolling?
Last edited 5 years ago by balaji (previous) (diff)

comment:4 Changed 5 years ago by balaji

  • Milestone changed from mpich-3.1 to future

comment:5 Changed 5 years ago by blocksom

I rebased the mpich-ibm/ticket-1837 branch to mpich/master.

comment:6 Changed 5 years ago by balaji

Mike: are you sure there are no differences that haven't yet been pushed into mpich-ibm/master?

This patch cannot even build without some other corresponding change to define a few functions. For example:

grep -r MPID_cc_prefetch *
src/mpi/pt2pt/waitall.c:        MPID_cc_prefetch_t  pre_cc;
src/mpi/pt2pt/waitall.c:                info0.pre_cc  = MPID_cc_prefetch(&info0.req->cc);
src/mpi/pt2pt/waitall.c:                info1.pre_cc  = MPID_cc_prefetch(&info1.req->cc);
src/mpi/pt2pt/waitall.c:            if ( likely((info0.req!=NULL) & ((info0.kind == MPID_REQUEST_SEND) | (info0.kind == MPID_REQUEST_RECV)) & MPID_cc_prefetch_is_complete(info0.pre_cc)) )
src/mpi/pt2pt/waitall.c:            if ( likely((info1.req!=NULL) & ((info1.kind == MPID_REQUEST_SEND) | (info1.kind == MPID_REQUEST_RECV)) & MPID_cc_prefetch_is_complete(info1.pre_cc)) )
src/mpi/pt2pt/waitall.c:                info0.pre_cc  = MPID_cc_prefetch(&info0.req->cc);
src/mpi/pt2pt/waitall.c:                if ( ((info0.kind == MPID_REQUEST_SEND) | (info0.kind == MPID_REQUEST_RECV)) && MPID_cc_prefetch_is_complete(info0.pre_cc) )

MPID_cc_prefetch is used, but not defined any where. How is mpich-ibm/build working with this patch? Did something get dropped in the BGQ/PE merge?

comment:7 Changed 5 years ago by blocksom

I'll have to check .. I think I just took the commit that was "skipped" by Dave a few months ago (!) and stuffed it into a ticket and branch. Things may have changed since then.

FWIW, I can build bgq mpich using both the mpich/master branch and the mpich-ibm/build branch.

comment:8 Changed 5 years ago by balaji

I did some more digging around with the various ticket branches and it looks like this ticket has a dependency on #1835, which defines the macros in question. We'll need to handle this a better way. Can you add the commit from #1835 before this patch in the mpich-ibm/ticket-1837 branch, so we don't miss the dependency in the future?

comment:9 Changed 5 years ago by blocksom

I pushed a new branch, mpich-ibm/ticket-1837-alt, which is based on the branch mpich-ibm/ticket-1835.

comment:10 Changed 5 years ago by blocksom

Using some git fu I was able to tease out the individual commits that composed the code differences that are discussed in this ticket.

There is a dependency on ticket #1835, so the mpich-ibm/ticket-1837-alt2 branch I've pushed is based on the branch mpich-ibm/ticket-1835-alt2.

comment:11 Changed 5 years ago by blocksom

I rebased this to master, pushed as branch mpich-ibm/ticket-1837, and deleted branch mpich-ibm/ticket-1837-alt2.

comment:12 Changed 4 years ago by blocksom

I rebased the code to the mpich-ibm/ticket-1835-v3.1rc4 branch and deleted the old ticket-1837 branch.

See mpich- ibm/ticket-1837-v3.1rc4.

comment:13 Changed 3 years ago by balaji

  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.