<div dir="ltr">Hi,<div><br></div><div>RVec was intended as a convenience feature for people writing high-level, rather than low-level code. I would indeed not advocate using it in performance-sensitive mdrun code without checking for such performance effects.</div><div><br></div><div>I think the real issue is closer to that the use of *rvec* generally confuses compilers because annotations about pointers do indeed get lost, and perhaps [3] in real [3][] is not as close to being part of the type in the compiler implementation as we might expect. (For a long time, we&#39;ve used very little rvec close to the short-ranged kernels. The first thing we do with the rvec * args to the SIMD bondeds is to cast them to real *. I think this is just as hard to police as Berk&#39;s concern about not using std::vector&lt;RVec&gt;::operator[] naively - the problem isn&#39;t the &quot;fault&quot; of std::vector or its use.)</div><div><br></div><div>The use of rvec * engenders some transposing that might be avoided if we had SoA rather than AoS layout in mdrun. A single rvec is likely to have all three coordinates on the same cache line, but if one is anyway often using nearby atoms the caching behaviour of three separate real * arrays should be rather similar. One still tends to have gather/scatter overhead, however.</div><div><br></div><div>Mark</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Mar 23, 2016 at 11:24 AM Berk Hess &lt;<a href="mailto:hess@kth.se">hess@kth.se</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF">
    <div>No.<br>
      using the RVec * pointer obtained through .data() already fixes
      the issue. We don&#39;t even need as_rvec_array().<br>
      <br>
      The real issue is that it&#39;s near impossible to enforce this and
      detect vector access of RVecs if we start using std::vector
      everywhere through the code.<br>
      <br>
      Cheers,<br>
      <br>
      Berk</div></div><div text="#000000" bgcolor="#FFFFFF"><div><br>
      <br>
      On 2016-03-23 11:19, Mark Abraham wrote:<br>
    </div></div><div text="#000000" bgcolor="#FFFFFF">
    <blockquote type="cite">
      
      <div dir="ltr">Hi,
        <div><br>
        </div>
        <div>If so, then we might want a helper function like
          as_rvec_array, but as_real_array instead.</div>
        <div><br>
        </div>
        <div>Mark</div>
      </div>
      <br>
      <div class="gmail_quote">
        <div dir="ltr">On Wed, Mar 23, 2016 at 10:51 AM Berk Hess &lt;<a href="mailto:hess@kth.se" target="_blank"><a href="mailto:hess@kth.se" target="_blank">hess@kth.se</a></a>&gt;
          wrote:<br>
        </div>
        <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
          <div text="#000000" bgcolor="#FFFFFF">
            <div>On 2016-03-23 10:50, Erik Lindahl wrote:<br>
            </div>
            <blockquote type="cite"> Hi,
              <div><br>
              </div>
              <div>I haven’t followed the discussion in detail, but a
                long time ago I remember having simliar issues in the
                kernels when using a list[] of rvec[] (in plain-old-c,
                no c++) instead of extracting the pointer and handling
                the multiply-by-3 manually. Could it be something
                similar here, e.g. that the compiler things it does not
                know enough about RVec rather than something going from
                with the outer list?</div>
            </blockquote>
          </div>
          <div text="#000000" bgcolor="#FFFFFF"> That would be my guess.
            The index used in the same loop comes from a vector as well
            and doesn&#39;t seem to affect performance.</div>
          <div text="#000000" bgcolor="#FFFFFF"><br>
            <br>
            Berk</div>
          <div text="#000000" bgcolor="#FFFFFF"><br>
            <blockquote type="cite">
              <div><br>
              </div>
              <div>Cheers,</div>
              <div><br>
              </div>
              <div>Erik</div>
              <div><br>
              </div>
              <div>
                <div>
                  <blockquote type="cite">
                    <div>On 23 Mar 2016, at 10:44, Berk Hess &lt;<a href="mailto:hess@kth.se" target="_blank"><a href="mailto:hess@kth.se" target="_blank">hess@kth.se</a></a>&gt; wrote:</div>
                    <br>
                    <div>
                      <div text="#000000" bgcolor="#FFFFFF">
                        <div>On 2016-03-23 10:42, Mark Abraham wrote:<br>
                        </div>
                        <blockquote type="cite">
                          <div dir="ltr">Hi,<br>
                            <br>
                            <div class="gmail_quote">
                              <div dir="ltr">On Wed, Mar 23, 2016 at
                                9:44 AM Berk Hess &lt;<a href="mailto:hess@kth.se" target="_blank"><a href="mailto:hess@kth.se" target="_blank">hess@kth.se</a></a>&gt;
                                wrote:<br>
                              </div>
                              <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
                                <br>
                                Luckily Szilard does thorough testing
                                and noticed a performance<br>
                                degradation in change set 25 of <a href="https://gerrit.gromacs.org/#/c/5232/" target="_blank"><a href="https://gerrit.gromacs.org/#/c/5232/" target="_blank">https://gerrit.gromacs.org/#/c/5232/</a></a>
                                The<br>
                                only signifcant change with respect to
                                previous sets is replacing C<br>
                                pointers by std::vector. I traced the
                                performance difference back to a<br>
                                single loop, which must have become
                                several factors slower to explain<br>
                                the time difference. I get the
                                performance back when replacing the<br>
                                vector by a pointer extracted with
                                .data(), see below. I looked at the<br>
                                assembly code from gcc 5.3.1 and the
                                vector case generated 200 extra<br>
                                instructions, which makes it difficult
                                to see what the actual difference<br>
                                is. The pointer case uses a lot of
                                vmovss and vaddss, which the vector<br>
                                one does much less, but this is only
                                serial SIMD instruction. I thought<br>
                                that [] in vector might does bounds
                                checking,</blockquote>
                              <div><br>
                              </div>
                              <div>Definitely not in release builds.</div>
                              <div> </div>
                              <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> but it
                                seems it does not.<br>
                                Can anyone explain why the vector case
                                can be so slow?</blockquote>
                              <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If this is
                                a general issue (with RVec or more?), we
                                need to always extra<br>
                                a pointer with .data() for use in all
                                inner-loops. This is pretty<br>
                                annoying and difficult to enforce.<br>
                                <br>
                                Cheers,<br>
                                <br>
                                Berk<br>
                                <br>
                                                        const
                                std::vector&lt;RVec&gt; f_foreign =<br>
                                idt_foreign-&gt;force<br>
                              </blockquote>
                              <div><br>
                              </div>
                              <div>This does a copy of the vector, and
                                doesn&#39;t seem to be in any version of
                                this patch in gerrit. Is this what you
                                meant to write?</div>
                            </div>
                          </div>
                        </blockquote>
                        I tried this. But my original &quot;vectorized&quot; patch
                        set took a pointer from idt_foreign and did not
                        copy the vector, that gives the same, slow,
                        performance.<br>
                        <br>
                        Berk<br>
                        <blockquote type="cite">
                          <div dir="ltr">
                            <div class="gmail_quote">
                              <div><br>
                              </div>
                              <div>Mark</div>
                              <div><br>
                              </div>
                              <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> or<br>
                                                        const RVec     
                                         *f_foreign   =<br>
                                idt_foreign-&gt;force.data();<br>
                                <br>
                                                         int natom =
                                atomList-&gt;atom.size();<br>
                                                         for (int i = 0;
                                i &lt; natom; i++)<br>
                                                         {<br>
                                                             int ind =
                                atomList-&gt;atom[i];<br>
                                                           
                                 rvec_inc(f[ind], f_foreign[ind]);<br>
                                                         }<br>
                                --<br>
                                Gromacs Developers mailing list<br>
                                <br>
                                * Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" target="_blank"><a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a></a>
                                before posting!<br>
                                <br>
                                * Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank"><a href="http://www.gromacs.org/Support/Mailing_Lists" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a></a><br>
                                <br>
                                * For (un)subscribe requests visit<br>
                                <a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a>
                                or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank"><a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a></a>.<br>
                              </blockquote>
                            </div>
                          </div>
                          <br>
                          <fieldset></fieldset>
                          <br>
                        </blockquote>
                        <br>
                      </div>
                      -- <br>
                      Gromacs Developers mailing list<br>
                      <br>
                      * Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" target="_blank"><a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a></a>
                      before posting!<br>
                      <br>
                      * Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
                      <br>
                      * For (un)subscribe requests visit<br>
                      <a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a>
                      or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</div>
                  </blockquote>
                </div>
                <br>
              </div>
              <br>
              <fieldset></fieldset>
              <br>
            </blockquote>
            <br>
          </div>
          --<br>
          Gromacs Developers mailing list<br>
          <br>
          * Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a>
          before posting!<br>
          <br>
          * Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
          <br>
          * For (un)subscribe requests visit<br>
          <a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a>
          or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</blockquote>
      </div>
      <br>
      <fieldset></fieldset>
      <br>
    </blockquote>
    <br>
  </div>

--<br>
Gromacs Developers mailing list<br>
<br>
* Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a> before posting!<br>
<br>
* Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
<br>
* For (un)subscribe requests visit<br>
<a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a> or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</blockquote></div>