<html>
  <head>
    <meta content="text/html; charset=windows-1252"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    I haven't heard any objection to the proposed attribute.  Given
    that, I think we can go ahead and move forward.<br>
    <br>
    Igor - Can you update the patch to use the name argmemonly
    (analogous to readonly, readnone)?  As discussed, we want to factor
    out the read/write part and use the existing attributes.  Make sure
    you update the docs to clarify the answer to Nick's question
    regarding following pointers.<br>
    <br>
    Once that's done, I'll take a close look and probably LGTM.  We will
    need to find someone to review the bitcode/IR serialization though
    since I'm not familiar with those pieces.  <br>
    <br>
    After this goes in, a good cleanup would be to remove the special
    casing for intrinsics and have the td files drive the addition of
    the now standardized attributes.  This would be both a good cleanup
    and ensure test coverage of the newly introduced attribute.  As
    such, I'd like you (Igor) to commit to doing this after the first
    patch lands.<br>
    <br>
    Philip<br>
    <br>
    <div class="moz-cite-prefix">On 06/19/2015 10:57 AM, Philip Reames
      wrote:<br>
    </div>
    <blockquote cite="mid:55845806.1030805@philipreames.com" type="cite">
      <meta content="text/html; charset=windows-1252"
        http-equiv="Content-Type">
      <br>
      <br>
      <div class="moz-cite-prefix">On 06/18/2015 09:15 PM, Nick Lewycky
        wrote:<br>
      </div>
      <blockquote
cite="mid:CADbEz-iDvXoia_8nHAnJNJnvWQuh1F6H0Wqy2WRq1ym6Sj8nkQ@mail.gmail.com"
        type="cite">
        <blockquote class="gmail_quote" style="margin:0 0 0
          .8ex;border-left:1px #ccc solid;padding-left:1ex">Currently in
          AliasAnalysis we can model ModRef behaviour for functions
          which<br>
          only access memory through their pointer arguments. However we
          can't<br>
          express this propery as a function attribute.<br>
          <br>
          For example, for intrinsics we can specify ReadWriteArgMem or
          ReadArgMem<br>
          attributes in tablegen definitions. But due to the lack of the
          related function<br>
          attributes on the llvm ir level, this intrinsics would be
          modelled as if they<br>
          were clobbering arbitrary memory.<br>
          <br>
          It feels very naturall to add new function attribute which can
          cover such cases.<br>
          <br>
          I have a patch (<a moz-do-not-send="true"
href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10398&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=6QD3kOt2shxvhEw0pkmPGL_NzzjWw6s3ZTzBo-rwkUs&s=MJrrF5wf5KOXiTUsfpoJMSDYvyM_nZ3L79ZSsA9iXaA&e="
            rel="noreferrer" target="_blank">http://reviews.llvm.org/D10398</a>)
          in which I added this<br>
          attribute. Currently there is some discussion on how to name
          it and how it should<br>
          behave when defined together with other fucntion attributes.<br>
        </blockquote>
        <div><br>
        </div>
        <div>What does it mean? Can it only touch memory that is
          directly referred to by an argument? Or if that argument
          points to another pointer, can we follow it?</div>
      </blockquote>
      I just want to point out that the notion Igor is introducing as an
      attribute is not a new one.  It's a prexisting notion which is
      already implemented within LLVM today; it's simply been restricted
      to intrinsics.  Here's the definition from Intrinsics.td:<br>
      // IntrReadWriteArgMem - This intrinsic reads and writes only from
      memory that                                                 <br>
      // one of its arguments points to, but may access an unspecified
      amount.  The                                                  <br>
      // reads and writes may be volatile, but except for this it has no
      other side                                                  <br>
      //
      effects.                                                                                                                   

      <br>
      def IntrReadWriteArgMem : IntrinsicProperty;   <br>
      <br>
      I did point out in the review that I think the notion of
      ReadsArgMem is redundant given the existing notions of
      ReadWriteArgMem and ReadOnly, but that's somewhat orthogonal. 
      It's true of the existing implementation, not just Igor's patch. 
      It may be worth settling on this to clarify naming (i.e. are we
      ever going to need an attribute like reads_arg_mem?  Or can we be
      more generic and use accesses_arg_mem + readonly for the same
      purpose?), but I don't think that really changes the semantics in
      major ways.  <br>
      <br>
      Philip<br>
      <br>
      <br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
      <pre wrap="">_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a class="moz-txt-link-freetext" href="http://llvm.cs.uiuc.edu">http://llvm.cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a>
</pre>
    </blockquote>
    <br>
  </body>
</html>