[PATCH] D15499: Add AlmostReadNone and AlmostArgMemOnly attributes and set it for a few libc functions. Enhance GlobalsAA

[Vaivaswatha Nagaraj via llvm-commits]

vaivaswatha added a comment.

In http://reviews.llvm.org/D15499#309992, @jmolloy wrote:

> Hi Vaivaswatha,
>
> Thanks for pushing this forward. Firstly, this patch should be split into at about four separate patches:
>
> 1. Introduce new function attributes
> 2. Modify GlobalsAA
> 3. Add these new attributes to libc functions
> 4. Add __mulsc3
>
>   The LLVM community prefer small, targetted patches, and each should come with at least one test case.


I can do that. So the first patch can be part of this review and I'll create new ones as I go along?

> There will be a lot of bikeshedding on the names for these attributes. I'll just throw my penniworth in the ring and say I don't like the current names. "almostreadnone" only tells me that it isn't readnone - I'm going to have to look at the LangRef (which needs updating as part of patch (1) by the way) to see what it means.

> 

> A well-named attribute should imply its semantics at least vaguely. "readnone" for example is self explanatory. It'd be good to replace "almost" with something that describes how it differs from "readnone".

> 

> I'd suggest having a look the commits that introduced other attributes (like "norecurse" that I added recently) - they will show you what tests need touching/adding. Adding an attribute requires more tests than a normal commit, because you change the bitcode format and we need to ensure we don't regress this.

> 

> Did you run the LLVM regression tests? I think this line:

> 

> > ATTR_KIND_ALMOST_ARGMEM_ONLY = 50,

> 

> 

> should have broken at least one test - there is an invalid bitcode test that uses "50" as a known-invalid attribute kind.


I did run the regression tests and as expected saw some failures. I am currently working on them. I'll update this patch with the corrected tests, and only covering work-item (1). Will submit more patches as it progresses.

> 

> 

> > /// @brief Determine if the function may read only non IR visible globals,

> 

> 

> What about heap allocations or any other allocation that is considered an object, like an alloca whose address has been taken? Are we restricting this completely to globals, or do we need to expand this to cover all addressable objects?


Does "read only non-IR visible memory" sound more appropriate ?

> Cheers,

> 

> James


I'll also update the langref when I submit the new patch.


Repository:
  rL LLVM

http://reviews.llvm.org/D15499