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

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

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.

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.

> /// @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?

Cheers,

James


Repository:
  rL LLVM

http://reviews.llvm.org/D15499