llvm dev - Feb 2016 - RFC: Remove inaccessiblememonly from 3.8 branch

A while back, we introduced new attributes to model functions which read 
and wrote from locations not otherwise visible in the IR.  The 
motivation was to enable much more aggressive aliasing around such calls.

Since that time, we've reverted the original patch and AFAIK we have no 
transformation or analysis passes in tree which use this information.  
Given this, I don't think it's really fair to say the attribute has 
baked at all.  We have no reason to believe that we picked the right 
semantics, and last I remember, there was some legitimate question about 
that.  Worse, AFAIK, all active development has stopped.  I don't know 
of any active reviews for this attribute.

These attributes have not yet shipped in any LLVM release, but this is 
about to change in 3.8.  Once that changes, we will have accepted a 
backwards compatibility guarantee with bitcode which is potentially 
concerning for a new attribute in such a half developed state.

Given the preceding, I'd like to propose that we remove the 
inaccessiblememonly and inaccessiblemem_or_argmemonly attributes from 
the 3.8 release branch.  This could be done by either a) actually 
removing the code, or b) simply disabling it.  If we remove the 
documentation from the LangRef, disable the LL and bitcode parsing for 
the attribute, and explicitly document that we are not reserving the 
attribute index numbers, I think that prevents any assumption of forward 
compatibility.  Another option would be to mark them experimental, which 
would have effectively the same effect.

Philip

The original GlobalsAA:
http://reviews.llvm.org/D15605

The revert:
http://reviews.llvm.org/D15919

The last active revision I know of: (Abandoned Jan 6)
http://reviews.llvm.org/D15665

Hi Philip
> On Feb 10, 2016, at 1:12 PM, Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> A while back, we introduced new attributes to model functions which read
and wrote from locations not otherwise visible in the IR.  The motivation was to
enable much more aggressive aliasing around such calls.
> 
> Since that time, we've reverted the original patch and AFAIK we have no
transformation or analysis passes in tree which use this information.  Given
this, I don't think it's really fair to say the attribute has baked at
all.  We have no reason to believe that we picked the right semantics, and last
I remember, there was some legitimate question about that.  Worse, AFAIK, all
active development has stopped.  I don't know of any active reviews for this
attribute.
> 
> These attributes have not yet shipped in any LLVM release, but this is
about to change in 3.8.  Once that changes, we will have accepted a backwards
compatibility guarantee with bitcode which is potentially concerning for a new
attribute in such a half developed state.
> 
> Given the preceding, I'd like to propose that we remove the
inaccessiblememonly and inaccessiblemem_or_argmemonly attributes from the 3.8
release branch.  This could be done by either a) actually removing the code, or
b) simply disabling it.  If we remove the documentation from the LangRef,
disable the LL and bitcode parsing for the attribute, and explicitly document
that we are not reserving the attribute index numbers, I think that prevents any
assumption of forward compatibility.  Another option would be to mark them
experimental, which would have effectively the same effect.
I don’t mind which of the above options we choose, but +1 for doing something.

The most important point to me is that of forward compatibility.  If we ship it
then I feel like we are more or less accepting the current semantics of the
attribute, even if that later turns out to not be what we want.

For that reason, I like that you pointed out attributes being experimental. 
Perhaps being experimental should be the default for new attributes (default
doesn’t mean we can’t add new non-experimental ones), as then at least we are
free to change attributes as needed.

The same thing kind of happened with convergent which was added but then changed
slightly in behaviour.  Luckily I don’t think we had a release in that timeframe
so it wasn’t a problem, but had we had a release with someone relying on the
initial behaviour of convergent, we could have had trouble changing it later.

Cheers,
Pete
> 
> Philip
> 
> The original GlobalsAA:
> http://reviews.llvm.org/D15605
> 
> The revert:
> http://reviews.llvm.org/D15919
> 
> The last active revision I know of: (Abandoned Jan 6)
> http://reviews.llvm.org/D15665
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

----- Original Message -----
> From: "Pete Cooper via llvm-dev" <llvm-dev at
lists.llvm.org>
> To: "Philip Reames" <listmail at philipreames.com>
> Cc: "llvm-dev" <llvm-dev at lists.llvm.org>
> Sent: Wednesday, February 10, 2016 3:29:32 PM
> Subject: Re: [llvm-dev] RFC: Remove inaccessiblememonly from 3.8 branch
> 
> Hi Philip
> > On Feb 10, 2016, at 1:12 PM, Philip Reames via llvm-dev
> > <llvm-dev at lists.llvm.org> wrote:
> > 
> > A while back, we introduced new attributes to model functions which
> > read and wrote from locations not otherwise visible in the IR.
> >  The motivation was to enable much more aggressive aliasing around
> > such calls.
> > 
> > Since that time, we've reverted the original patch and AFAIK we
> > have no transformation or analysis passes in tree which use this
> > information.  Given this, I don't think it's really fair to
say
> > the attribute has baked at all.  We have no reason to believe that
> > we picked the right semantics, and last I remember, there was some
> > legitimate question about that.  Worse, AFAIK, all active
> > development has stopped.  I don't know of any active reviews for
> > this attribute.
> > 
> > These attributes have not yet shipped in any LLVM release, but this
> > is about to change in 3.8.  Once that changes, we will have
> > accepted a backwards compatibility guarantee with bitcode which is
> > potentially concerning for a new attribute in such a half
> > developed state.
> > 
> > Given the preceding, I'd like to propose that we remove the
> > inaccessiblememonly and inaccessiblemem_or_argmemonly attributes
> > from the 3.8 release branch.  This could be done by either a)
> > actually removing the code, or b) simply disabling it.  If we
> > remove the documentation from the LangRef, disable the LL and
> > bitcode parsing for the attribute, and explicitly document that we
> > are not reserving the attribute index numbers, I think that
> > prevents any assumption of forward compatibility.  Another option
> > would be to mark them experimental, which would have effectively
> > the same effect.
> I don’t mind which of the above options we choose, but +1 for doing
> something.
+1

 -Hal
> 
> The most important point to me is that of forward compatibility.  If
> we ship it then I feel like we are more or less accepting the
> current semantics of the attribute, even if that later turns out to
> not be what we want.
> 
> For that reason, I like that you pointed out attributes being
> experimental.  Perhaps being experimental should be the default for
> new attributes (default doesn’t mean we can’t add new
> non-experimental ones), as then at least we are free to change
> attributes as needed.
> 
> The same thing kind of happened with convergent which was added but
> then changed slightly in behaviour.  Luckily I don’t think we had a
> release in that timeframe so it wasn’t a problem, but had we had a
> release with someone relying on the initial behaviour of convergent,
> we could have had trouble changing it later.
> 
> Cheers,
> Pete
> > 
> > Philip
> > 
> > The original GlobalsAA:
> > http://reviews.llvm.org/D15605
> > 
> > The revert:
> > http://reviews.llvm.org/D15919
> > 
> > The last active revision I know of: (Abandoned Jan 6)
> > http://reviews.llvm.org/D15665
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory

> On Feb 10, 2016, at 1:12 PM, Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> A while back, we introduced new attributes to model functions which read
and wrote from locations not otherwise visible in the IR.  The motivation was to
enable much more aggressive aliasing around such calls.
> 
> Since that time, we've reverted the original patch
inaccessiblememonly is still in trunk as of today right? You're referring to
the GlobalsAA one right?
> and AFAIK we have no transformation or analysis passes in tree which use
this information.  Given this, I don't think it's really fair to say the
attribute has baked at all.  We have no reason to believe that we picked the
right semantics, and last I remember, there was some legitimate question about
that.  Worse, AFAIK, all active development has stopped.  I don't know of
any active reviews for this attribute.
> 
> These attributes have not yet shipped in any LLVM release, but this is
about to change in 3.8.  Once that changes, we will have accepted a backwards
compatibility guarantee with bitcode which is potentially concerning for a new
attribute in such a half developed state.
It seems to me that the attribute can be just dropped safely when reading
bitcode without correctness issue right? The consequence would be at worse a
missing optimization or could it be worse?
> 
> Given the preceding, I'd like to propose that we remove the
inaccessiblememonly and inaccessiblemem_or_argmemonly attributes from the 3.8
release branch.  This could be done by either a) actually removing the code, or
b) simply disabling it.  If we remove the documentation from the LangRef,
disable the LL and bitcode parsing for the attribute, and explicitly document
that we are not reserving the attribute index numbers, I think that prevents any
assumption of forward compatibility.  Another option would be to mark them
experimental, which would have effectively the same effect.
Given it has never shipped and is not used anywhere (other than
reader/parser/writer), I agree with you that there is no reason to ship the
attribute for 3.8 and we should remove it.

-- 
Mehdi