llvm dev - Feb 2016 - Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")

----- Original Message -----
> From: "Chandler Carruth" <chandlerc at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-dev" <llvm-dev at lists.llvm.org>, "Philip
Reames"
> <listmail at philipreames.com>, "Duncan P. N. Exon Smith"
> <dexonsmith at apple.com>, "Xinliang David Li"
<xinliangli at gmail.com>,
> "Sanjoy Das" <sanjoy at playingwithpointers.com>
> Sent: Friday, February 26, 2016 9:41:23 PM
> Subject: Re: [llvm-dev] Possible soundness issue with
> available_externally (split from "RFC: Add guard intrinsics")
> On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel < hfinkel at anl.gov >
wrote:
> > > From: "Chandler Carruth" < chandlerc at google.com
>
> > 
> 
> > > To: "Hal Finkel" < hfinkel at anl.gov >
> > 
> 
> > > Cc: "llvm-dev" < llvm-dev at lists.llvm.org >,
"Philip Reames" <
> > > listmail at philipreames.com >, "Duncan P. N. Exon
Smith" <
> > > dexonsmith at apple.com >, "Xinliang David Li" <
> > > xinliangli at gmail.com
> > > >, "Sanjoy Das" < sanjoy at
playingwithpointers.com >
> > 
> 
> > > Sent: Friday, February 26, 2016 9:33:55 PM
> > 
> 
> > > Subject: Re: [llvm-dev] Possible soundness issue with
> > > available_externally (split from "RFC: Add guard
intrinsics")
> > 
> 
> > > On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel < hfinkel at
anl.gov >
> > > wrote:
> > 
> 
> > > > > From: "Chandler Carruth" < chandlerc at
google.com >
> > > 
> > 
> 
> > > > > To: "Hal Finkel" < hfinkel at anl.gov
>, "Sanjoy Das" <
> > > > > sanjoy at playingwithpointers.com >
> > > 
> > 
> 
> > > > > Cc: "llvm-dev" < llvm-dev at
lists.llvm.org >, "Philip Reames" <
> > > > > listmail at philipreames.com >, "Duncan P. N.
Exon Smith"
> > > 
> > 
> 
> > > > > < dexonsmith at apple.com >, "Xinliang David
Li" <
> > > > > xinliangli at gmail.com >
> > > 
> > 
> 
> > > > > Sent: Friday, February 26, 2016 9:01:48 PM
> > > 
> > 
> 
> > > > > Subject: Re: [llvm-dev] Possible soundness issue with
> > > > > available_externally (split from "RFC: Add guard
intrinsics")
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > I think this will have a much higher cost than my
proposal to
> > > 
> > 
> 
> > > > > constrain how we deduce function attributes (which
still
> > > > > fixes
> > > 
> > 
> 
> > > > > Sanjoy's latest example).
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > Specifically, I think this will force us to constrain
far too
> > > > > many
> > > 
> > 
> 
> > > > > transformations for the sake of code size in functions
that
> > > > > we
> > > > > won't
> > > 
> > 
> 
> > > > > inline. Even if we were never going to deduce function
> > > > > attributes
> > > 
> > 
> 
> > > > > for anything in the function (because its big and reads
and
> > > > > writes
> > > 
> > 
> 
> > > > > everything), we'll still have to constrain our
> > > > > transformations
> > > > > just
> > > 
> > 
> 
> > > > > because we *might* later deduce a function attribute
that
> > > > > triggers
> > > 
> > 
> 
> > > > > these kinds of bugs.
> > > 
> > 
> 
> > > > >
> > > 
> > 
> 
> > > > > Essentially, you're proposing to limit
intraprocedural
> > > > > optimization
> > > 
> > 
> 
> > > > > to when we can successfully to interprocedural
optimization
> > > 
> > 
> 
> > > > > ("privatization"), where I'm suggesting
we limit
> > > > > interprocedural
> > > 
> > 
> 
> > > > > optimization to leave intraprocedural optimization
> > > > > unconstrained.
> > > 
> > 
> 
> > > > > Given the ratio of our optimizations (almost all are
intra,
> > > > > very
> > > > > few
> > > 
> > 
> 
> > > > > are inter), I'm much more comfortable with the
latter.
> > > 
> > 
> 
> > > > This is a good point; we can certainly (easily) delay the
> > > > privatization decision until we modify any IPA-level
function
> > > > information (at which point we can either reject the
attribute
> > > > change (when optimizing for code size), or keep it locally
> > > > (when
> > > > optimizing for speed). Ideally, you'd want to delay this
even
> > > > further (until you knew the attribute information was used),
> > > > but
> > > > I'm
> > > > not sure that's practical.
> > > 
> > 
> 
> > > > Actually, what if instead of actually privatizing, we moved
the
> > > > function into a different comdat section named after some
hash
> > > > of
> > > > the function body? That way, if all versions are actually
> > > > optimized
> > > > identically, we'll still only end up with one copy in
the final
> > > > executable. If this is technically possible, it seems like
the
> > > > best
> > > > kind of solution.
> > > 
> > 
> 
> > > This is how I want to do a revamped function merging anyways and
> > > it
> > > would fall out naturally of that.
> > 
> 
> > Excellent, so let's fix this at the same time we implement this
> > function merging (so that we don't have performance regressions in
> > an intermediate state). This will also allow us to have uniform
> > logic across different optimization levels, which is obviously
> > preferable.
> 
> I am *extremely* uncomfortable waiting to fix this until merging
> stuff is in place and we add privatization heuristics to our IPO
> passes. Those might be years away.
I'm *extremely* uncomfortable fixing this at all unless it can be done
without causing performance regressions. The underlying basic use case (linking
together code compiled with different optimization levels), is certainly
something I'd like to work properly, but is definitely a far lower priority
than optimized code quality and size.

Furthermore, you agree that there is a technical solution that satisfies these
requirements (placing functions into their own hashed comdat sections), and I
don't see why this is not relatively-straightforward to implement, and so if
we want to fix this bug, we should implement it. In the common case (where
everything is optimized at the same level), I don't see why it has any
additional overhead. We should be able to privatize aggressively under this
scheme.

-Hal 
> I think we should pretty immediately:
> 1) Make our IPO passes conservatively correc
> 2) Leave comments about how to add privatization, but explain the
> code size cost incurred by it
> I have no idea if anyone is even working on privatization (I'm not)
> or function merging (no ETA at all, its really far down my queue). I
> think we should decouple all these pieces. Once things are correct,
> folks can add a very size-conservative privatization transformation
> to IPO routines if we don't have merging, or a fairly aggressive one
> if we do have merging. And if we add merging later we can re-tune
> the privatization.
> -Chandler
> > Thanks again,
> 
> > Hal
> 
> > --
> 
> > Hal Finkel
> 
> > Assistant Computational Scientist
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 
-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160226/69641793/attachment-0001.html>

On Fri, Feb 26, 2016 at 7:59 PM Hal Finkel <hfinkel at anl.gov> wrote:
> *From: *"Chandler Carruth" <chandlerc at google.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org>, "Philip
Reames" <
> listmail at philipreames.com>, "Duncan P. N. Exon Smith" <
> dexonsmith at apple.com>, "Xinliang David Li" <xinliangli
at gmail.com>,
> "Sanjoy Das" <sanjoy at playingwithpointers.com>
>
> *Sent: *Friday, February 26, 2016 9:41:23 PM
>
>
> *Subject: *Re: [llvm-dev] Possible soundness issue with
> available_externally (split from "RFC: Add guard intrinsics")
>
> On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel <hfinkel at anl.gov>
wrote:
>
>> *From: *"Chandler Carruth" <chandlerc at google.com>
>> *To: *"Hal Finkel" <hfinkel at anl.gov>
>>
>> *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org>,
"Philip Reames" <
>> listmail at philipreames.com>, "Duncan P. N. Exon Smith"
<
>> dexonsmith at apple.com>, "Xinliang David Li"
<xinliangli at gmail.com>,
>> "Sanjoy Das" <sanjoy at playingwithpointers.com>
>> *Sent: *Friday, February 26, 2016 9:33:55 PM
>>
>>
>> *Subject: *Re: [llvm-dev] Possible soundness issue with
>> available_externally (split from "RFC: Add guard intrinsics")
>>
>> On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel <hfinkel at anl.gov>
wrote:
>>
>>
>>> > From: "Chandler Carruth" <chandlerc at
google.com>
>>> > To: "Hal Finkel" <hfinkel at anl.gov>,
"Sanjoy Das" <
>>> sanjoy at playingwithpointers.com>
>>> > Cc: "llvm-dev" <llvm-dev at lists.llvm.org>,
"Philip Reames" <
>>> listmail at philipreames.com>, "Duncan P. N. Exon
Smith"
>>> > <dexonsmith at apple.com>, "Xinliang David Li"
<xinliangli at gmail.com>
>>> > Sent: Friday, February 26, 2016 9:01:48 PM
>>> > Subject: Re: [llvm-dev] Possible soundness issue with
>>> available_externally (split from "RFC: Add guard
intrinsics")
>>> >
>>> >
>>> > I think this will have a much higher cost than my proposal to
>>> > constrain how we deduce function attributes (which still fixes
>>> > Sanjoy's latest example).
>>> >
>>> > Specifically, I think this will force us to constrain far too
many
>>> > transformations for the sake of code size in functions that we
won't
>>> > inline. Even if we were never going to deduce function
attributes
>>> > for anything in the function (because its big and reads and
writes
>>> > everything), we'll still have to constrain our
transformations just
>>> > because we *might* later deduce a function attribute that
triggers
>>> > these kinds of bugs.
>>> >
>>> > Essentially, you're proposing to limit intraprocedural
optimization
>>> > to when we can successfully to interprocedural optimization
>>> > ("privatization"), where I'm suggesting we limit
interprocedural
>>> > optimization to leave intraprocedural optimization
unconstrained.
>>> > Given the ratio of our optimizations (almost all are intra,
very few
>>> > are inter), I'm much more comfortable with the latter.
>>>
>>> This is a good point; we can certainly (easily) delay the
privatization
>>> decision until we modify any IPA-level function information (at
which point
>>> we can either reject the attribute change (when optimizing for code
size),
>>> or keep it locally (when optimizing for speed). Ideally, you'd
want to
>>> delay this even further (until you knew the attribute information
was
>>> used), but I'm not sure that's practical.
>>>
>>> Actually, what if instead of actually privatizing, we moved the
function
>>> into a different comdat section named after some hash of the
function body?
>>> That way, if all versions are actually optimized identically,
we'll still
>>> only end up with one copy in the final executable. If this is
technically
>>> possible, it seems like the best kind of solution.
>>>
>>
>> This is how I want to do a revamped function merging anyways and it
would
>> fall out naturally of that.
>>
>> Excellent, so let's fix this at the same time we implement this
function
>> merging (so that we don't have performance regressions in an
intermediate
>> state). This will also allow us to have uniform logic across different
>> optimization levels, which is obviously preferable.
>>
>
> I am *extremely* uncomfortable waiting to fix this until merging stuff is
> in place and we add privatization heuristics to our IPO passes. Those might
> be years away.
>
> I'm *extremely* uncomfortable fixing this at all unless it can be done
> without causing performance regressions. The underlying basic use case
> (linking together code compiled with different optimization levels), is
> certainly something I'd like to work properly, but is definitely a far
> lower priority than optimized code quality and size.
>
Ok, I prioritize this differently: it is absolutely critical. Every binary
I have includes files that fit this description. Every single binary.

I think that we will discover subtle miscompiles every time we try to make
function attributes more powerful until this is fixed. I think this is
absolutely critical to fix.

I'm fine if you want an option to enable unsafe behavior here, but I think
we have to have an option that provides correct results and I frankly think
it should be the default.

>
> Furthermore, you agree that there is a technical solution that satisfies
> these requirements (placing functions into their own hashed comdat
> sections),
>
No, I think that is a significant overstatement. I think that this would
require a pretty sizable amount of work to get the size regression to be
tolerable. I don't know how much work, nor when it could be done.

> and I don't see why this is not relatively-straightforward to
implement,
> and so if we want to fix this bug, we should implement it. In the common
> case (where everything is optimized at the same level), I don't see why
it
> has any additional overhead. We should be able to privatize aggressively
> under this scheme.
>
I don't have any confidence in our ability to perfectly merge privatized
routines. Maybe you do, but I genuinely do not how to do it with sufficient
reliability today.

I have ideas about how to get a *good* approximation that I think will
recover much of the penalty. But I think it would still need a size
threshold....

>
>
>  -Hal
>
>
> I think we should pretty immediately:
>
> 1) Make our IPO passes conservatively correc
> 2) Leave comments about how to add privatization, but explain the code
> size cost incurred by it
>
> I have no idea if anyone is even working on privatization (I'm not) or
> function merging (no ETA at all, its really far down my queue). I think we
> should decouple all these pieces. Once things are correct, folks can add a
> very size-conservative privatization transformation to IPO routines if we
> don't have merging, or a fairly aggressive one if we do have merging.
And
> if we add merging later we can re-tune the privatization.
>
> -Chandler
>
>
>>
>> Thanks again,
>> Hal
>>
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160227/021496db/attachment.html>

----- Original Message -----
> From: "Chandler Carruth" <chandlerc at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-dev" <llvm-dev at lists.llvm.org>, "Philip
Reames"
> <listmail at philipreames.com>, "Duncan P. N. Exon Smith"
> <dexonsmith at apple.com>, "Xinliang David Li"
<xinliangli at gmail.com>,
> "Sanjoy Das" <sanjoy at playingwithpointers.com>
> Sent: Friday, February 26, 2016 10:04:44 PM
> Subject: Re: [llvm-dev] Possible soundness issue with
> available_externally (split from "RFC: Add guard intrinsics")
> On Fri, Feb 26, 2016 at 7:59 PM Hal Finkel < hfinkel at anl.gov >
wrote:
> > > From: "Chandler Carruth" < chandlerc at google.com
>
> > 
> 
> > > To: "Hal Finkel" < hfinkel at anl.gov >
> > 
> 
> > > Cc: "llvm-dev" < llvm-dev at lists.llvm.org >,
"Philip Reames" <
> > > listmail at philipreames.com >, "Duncan P. N. Exon
Smith" <
> > > dexonsmith at apple.com >, "Xinliang David Li" <
> > > xinliangli at gmail.com
> > > >, "Sanjoy Das" < sanjoy at
playingwithpointers.com >
> > 
> 
> > > Sent: Friday, February 26, 2016 9:41:23 PM
> > 
> 
> > > Subject: Re: [llvm-dev] Possible soundness issue with
> > > available_externally (split from "RFC: Add guard
intrinsics")
> > 
> 
> > > On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel < hfinkel at
anl.gov >
> > > wrote:
> > 
> 
> > > > > From: "Chandler Carruth" < chandlerc at
google.com >
> > > > 
> > > 
> > 
> 
> > > > > To: "Hal Finkel" < hfinkel at anl.gov >
> > > > 
> > > 
> > 
> 
> > > > > Cc: "llvm-dev" < llvm-dev at
lists.llvm.org >, "Philip Reames" <
> > > > > listmail at philipreames.com >, "Duncan P. N.
Exon Smith" <
> > > > > dexonsmith at apple.com >, "Xinliang David
Li" <
> > > > > xinliangli at gmail.com
> > > > > >, "Sanjoy Das" < sanjoy at
playingwithpointers.com >
> > > > 
> > > 
> > 
> 
> > > > > Sent: Friday, February 26, 2016 9:33:55 PM
> > > > 
> > > 
> > 
> 
> > > > > Subject: Re: [llvm-dev] Possible soundness issue with
> > > > > available_externally (split from "RFC: Add guard
intrinsics")
> > > > 
> > > 
> > 
> 
> > > > > On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel < hfinkel
at anl.gov
> > > > > >
> > > > > wrote:
> > > > 
> > > 
> > 
> 
> > > > > > > From: "Chandler Carruth" <
chandlerc at google.com >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > To: "Hal Finkel" < hfinkel at
anl.gov >, "Sanjoy Das" <
> > > > > > > sanjoy at playingwithpointers.com >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Cc: "llvm-dev" < llvm-dev at
lists.llvm.org >, "Philip
> > > > > > > Reames"
> > > > > > > <
> > > > > > > listmail at philipreames.com >,
"Duncan P. N. Exon Smith"
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > < dexonsmith at apple.com >,
"Xinliang David Li" <
> > > > > > > xinliangli at gmail.com >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Sent: Friday, February 26, 2016 9:01:48 PM
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Subject: Re: [llvm-dev] Possible soundness
issue with
> > > > > > > available_externally (split from "RFC:
Add guard
> > > > > > > intrinsics")
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > I think this will have a much higher cost
than my
> > > > > > > proposal
> > > > > > > to
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > constrain how we deduce function attributes
(which still
> > > > > > > fixes
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Sanjoy's latest example).
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Specifically, I think this will force us to
constrain far
> > > > > > > too
> > > > > > > many
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > transformations for the sake of code size in
functions
> > > > > > > that
> > > > > > > we
> > > > > > > won't
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > inline. Even if we were never going to deduce
function
> > > > > > > attributes
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > for anything in the function (because its big
and reads
> > > > > > > and
> > > > > > > writes
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > everything), we'll still have to
constrain our
> > > > > > > transformations
> > > > > > > just
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > because we *might* later deduce a function
attribute that
> > > > > > > triggers
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > these kinds of bugs.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Essentially, you're proposing to limit
intraprocedural
> > > > > > > optimization
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > to when we can successfully to
interprocedural
> > > > > > > optimization
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > ("privatization"), where I'm
suggesting we limit
> > > > > > > interprocedural
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > optimization to leave intraprocedural
optimization
> > > > > > > unconstrained.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > Given the ratio of our optimizations (almost
all are
> > > > > > > intra,
> > > > > > > very
> > > > > > > few
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > are inter), I'm much more comfortable
with the latter.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > This is a good point; we can certainly (easily)
delay the
> > > > > > privatization decision until we modify any
IPA-level
> > > > > > function
> > > > > > information (at which point we can either reject
the
> > > > > > attribute
> > > > > > change (when optimizing for code size), or keep it
locally
> > > > > > (when
> > > > > > optimizing for speed). Ideally, you'd want to
delay this
> > > > > > even
> > > > > > further (until you knew the attribute information
was
> > > > > > used),
> > > > > > but
> > > > > > I'm
> > > > > > not sure that's practical.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > Actually, what if instead of actually privatizing,
we moved
> > > > > > the
> > > > > > function into a different comdat section named
after some
> > > > > > hash
> > > > > > of
> > > > > > the function body? That way, if all versions are
actually
> > > > > > optimized
> > > > > > identically, we'll still only end up with one
copy in the
> > > > > > final
> > > > > > executable. If this is technically possible, it
seems like
> > > > > > the
> > > > > > best
> > > > > > kind of solution.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > This is how I want to do a revamped function merging
anyways
> > > > > and
> > > > > it
> > > > > would fall out naturally of that.
> > > > 
> > > 
> > 
> 
> > > > Excellent, so let's fix this at the same time we
implement this
> > > > function merging (so that we don't have performance
regressions
> > > > in
> > > > an intermediate state). This will also allow us to have
uniform
> > > > logic across different optimization levels, which is
obviously
> > > > preferable.
> > > 
> > 
> 
> > > I am *extremely* uncomfortable waiting to fix this until merging
> > > stuff is in place and we add privatization heuristics to our IPO
> > > passes. Those might be years away.
> > 
> 
> > I'm *extremely* uncomfortable fixing this at all unless it can be
> > done without causing performance regressions. The underlying basic
> > use case (linking together code compiled with different
> > optimization
> > levels), is certainly something I'd like to work properly, but is
> > definitely a far lower priority than optimized code quality and
> > size.
> 
> Ok, I prioritize this differently: it is absolutely critical. Every
> binary I have includes files that fit this description. Every single
> binary.
> I think that we will discover subtle miscompiles every time we try to
> make function attributes more powerful until this is fixed. I think
> this is absolutely critical to fix.
> I'm fine if you want an option to enable unsafe behavior here, but I
> think we have to have an option that provides correct results and I
> frankly think it should be the default.
I'm fine with this (i.e. there must be an option to turn it off). 
> > Furthermore, you agree that there is a technical solution that
> > satisfies these requirements (placing functions into their own
> > hashed comdat sections),
> 
> No, I think that is a significant overstatement. I think that this
> would require a pretty sizable amount of work to get the size
> regression to be tolerable. I don't know how much work, nor when it
> could be done.
Why would there be size regressions? Are there section alignment padding
concerns?
> > and I don't see why this is not relatively-straightforward to
> > implement, and so if we want to fix this bug, we should implement
> > it. In the common case (where everything is optimized at the same
> > level), I don't see why it has any additional overhead. We should
> > be
> > able to privatize aggressively under this scheme.
> 
> I don't have any confidence in our ability to perfectly merge
> privatized routines. Maybe you do, but I genuinely do not how to do
> it with sufficient reliability today.
> I have ideas about how to get a *good* approximation that I think
> will recover much of the penalty. But I think it would still need a
> size threshold....
Can you explain your concerns about reliability? My idea is that we have a kind
of delayed-name comdat section attribute that says that the name of the comdat
section into which the function will be placed will be based on the hash of the
contents (which should probably be the pre-isel IR-level contexts plus perhaps
the other state that might affect backend optimization). Maybe just using the
MI-level function contents will work too. It seems like this should be
"perfect". I don't see the circumstances why function definitions
optimized under identical optimization settings should differ in this sense.

Doing further, since our optimizations should be deterministic, we might even be
able to just hash the optimization level information and other relevant
attributes (which could affect TTI), and then be done with it.

Thanks again, 
Hal 
> > -Hal
> 
> > > I think we should pretty immediately:
> > 
> 
> > > 1) Make our IPO passes conservatively correc
> > 
> 
> > > 2) Leave comments about how to add privatization, but explain the
> > > code size cost incurred by it
> > 
> 
> > > I have no idea if anyone is even working on privatization
(I'm
> > > not)
> > > or function merging (no ETA at all, its really far down my
> > > queue).
> > > I
> > > think we should decouple all these pieces. Once things are
> > > correct,
> > > folks can add a very size-conservative privatization
> > > transformation
> > > to IPO routines if we don't have merging, or a fairly
aggressive
> > > one
> > > if we do have merging. And if we add merging later we can re-tune
> > > the privatization.
> > 
> 
> > > -Chandler
> > 
> 
> > > > Thanks again,
> > > 
> > 
> 
> > > > Hal
> > > 
> > 
> 
> > > > --
> > > 
> > 
> 
> > > > Hal Finkel
> > > 
> > 
> 
> > > > Assistant Computational Scientist
> > > 
> > 
> 
> > > > Leadership Computing Facility
> > > 
> > 
> 
> > > > Argonne National Laboratory
> > > 
> > 
> 
> > --
> 
> > Hal Finkel
> 
> > Assistant Computational Scientist
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 
-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160226/5ebbe960/attachment.html>

On Fri, Feb 26, 2016 at 7:59 PM, Hal Finkel <hfinkel at anl.gov>
wrote:
> I'm *extremely* uncomfortable fixing this at all unless it can be done
> without causing performance regressions. The underlying basic use case
> (linking together code compiled with different optimization levels), is
> certainly something I'd like to work properly, but is definitely a far
lower
> priority than optimized code quality and size.
You don't need different optimization levels to trigger this.  The
original example could have been

  void foo() available_externally {
    %t0 = load atomic %ptr
    %t1 = call i32 @bar()
    if (%t0 != %t1) print("X");
  }
  void main() {
    foo();
    print("Y");
  }

with @bar (just atomic_loads from %ptr and returns the value loaded)
being a declaration in one TU and a definition in another TU.  The TU
with the defn. could then inline @bar into @foo() while the one with
the decl. would not; leading to the same problem.

-- Sanjoy

On Fri, Feb 26, 2016 at 7:59 PM, Hal Finkel via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> ------------------------------
>
> *From: *"Chandler Carruth" <chandlerc at google.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org>, "Philip
Reames" <
> listmail at philipreames.com>, "Duncan P. N. Exon Smith" <
> dexonsmith at apple.com>, "Xinliang David Li" <xinliangli
at gmail.com>,
> "Sanjoy Das" <sanjoy at playingwithpointers.com>
> *Sent: *Friday, February 26, 2016 9:41:23 PM
>
> *Subject: *Re: [llvm-dev] Possible soundness issue with
> available_externally (split from "RFC: Add guard intrinsics")
>
> On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel <hfinkel at anl.gov>
wrote:
>
>> *From: *"Chandler Carruth" <chandlerc at google.com>
>> *To: *"Hal Finkel" <hfinkel at anl.gov>
>>
>> *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org>,
"Philip Reames" <
>> listmail at philipreames.com>, "Duncan P. N. Exon Smith"
<
>> dexonsmith at apple.com>, "Xinliang David Li"
<xinliangli at gmail.com>,
>> "Sanjoy Das" <sanjoy at playingwithpointers.com>
>> *Sent: *Friday, February 26, 2016 9:33:55 PM
>>
>>
>> *Subject: *Re: [llvm-dev] Possible soundness issue with
>> available_externally (split from "RFC: Add guard intrinsics")
>>
>> On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel <hfinkel at anl.gov>
wrote:
>>
>>
>>> > From: "Chandler Carruth" <chandlerc at
google.com>
>>> > To: "Hal Finkel" <hfinkel at anl.gov>,
"Sanjoy Das" <
>>> sanjoy at playingwithpointers.com>
>>> > Cc: "llvm-dev" <llvm-dev at lists.llvm.org>,
"Philip Reames" <
>>> listmail at philipreames.com>, "Duncan P. N. Exon
Smith"
>>> > <dexonsmith at apple.com>, "Xinliang David Li"
<xinliangli at gmail.com>
>>> > Sent: Friday, February 26, 2016 9:01:48 PM
>>> > Subject: Re: [llvm-dev] Possible soundness issue with
>>> available_externally (split from "RFC: Add guard
intrinsics")
>>> >
>>> >
>>> > I think this will have a much higher cost than my proposal to
>>> > constrain how we deduce function attributes (which still fixes
>>> > Sanjoy's latest example).
>>> >
>>> > Specifically, I think this will force us to constrain far too
many
>>> > transformations for the sake of code size in functions that we
won't
>>> > inline. Even if we were never going to deduce function
attributes
>>> > for anything in the function (because its big and reads and
writes
>>> > everything), we'll still have to constrain our
transformations just
>>> > because we *might* later deduce a function attribute that
triggers
>>> > these kinds of bugs.
>>> >
>>> > Essentially, you're proposing to limit intraprocedural
optimization
>>> > to when we can successfully to interprocedural optimization
>>> > ("privatization"), where I'm suggesting we limit
interprocedural
>>> > optimization to leave intraprocedural optimization
unconstrained.
>>> > Given the ratio of our optimizations (almost all are intra,
very few
>>> > are inter), I'm much more comfortable with the latter.
>>>
>>> This is a good point; we can certainly (easily) delay the
privatization
>>> decision until we modify any IPA-level function information (at
which point
>>> we can either reject the attribute change (when optimizing for code
size),
>>> or keep it locally (when optimizing for speed). Ideally, you'd
want to
>>> delay this even further (until you knew the attribute information
was
>>> used), but I'm not sure that's practical.
>>>
>>> Actually, what if instead of actually privatizing, we moved the
function
>>> into a different comdat section named after some hash of the
function body?
>>> That way, if all versions are actually optimized identically,
we'll still
>>> only end up with one copy in the final executable. If this is
technically
>>> possible, it seems like the best kind of solution.
>>>
>>
>> This is how I want to do a revamped function merging anyways and it
would
>> fall out naturally of that.
>>
>> Excellent, so let's fix this at the same time we implement this
function
>> merging (so that we don't have performance regressions in an
intermediate
>> state). This will also allow us to have uniform logic across different
>> optimization levels, which is obviously preferable.
>>
>
> I am *extremely* uncomfortable waiting to fix this until merging stuff is
> in place and we add privatization heuristics to our IPO passes. Those might
> be years away.
>
> I'm *extremely* uncomfortable fixing this at all unless it can be done
> without causing performance regressions. The underlying basic use case
> (linking together code compiled with different optimization levels), is
> certainly something I'd like to work properly, but is definitely a far
> lower priority than optimized code quality and size.
>
> Furthermore, you agree that there is a technical solution that satisfies
> these requirements (placing functions into their own hashed comdat
> sections), and I don't see why this is not relatively-straightforward
to
> implement, and so if we want to fix this bug, we should implement it. In
> the common case (where everything is optimized at the same level), I
don't
> see why it has any additional overhead. We should be able to privatize
> aggressively under this scheme.
>
Aren't we already getting some amount of deduping for free though from the
linker? (for linkers that support that)
I.e. if we actually *do* optimize it to the same machine code then
linker-level deduping will be able to eliminate it (in theory).

-- Sean Silva

>
>  -Hal
>
>
> I think we should pretty immediately:
>
> 1) Make our IPO passes conservatively correc
> 2) Leave comments about how to add privatization, but explain the code
> size cost incurred by it
>
> I have no idea if anyone is even working on privatization (I'm not) or
> function merging (no ETA at all, its really far down my queue). I think we
> should decouple all these pieces. Once things are correct, folks can add a
> very size-conservative privatization transformation to IPO routines if we
> don't have merging, or a fairly aggressive one if we do have merging.
And
> if we add merging later we can re-tune the privatization.
>
> -Chandler
>
>
>>
>> Thanks again,
>> Hal
>>
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160226/e498bbee/attachment-0001.html>

On 02/26/2016 08:04 PM, Chandler Carruth wrote:
> On Fri, Feb 26, 2016 at 7:59 PM Hal Finkel <hfinkel at anl.gov 
> <mailto:hfinkel at anl.gov>> wrote:
>
>         *From: *"Chandler Carruth" <chandlerc at google.com
>         <mailto:chandlerc at google.com>>
>         *To: *"Hal Finkel" <hfinkel at anl.gov
<mailto:hfinkel at anl.gov>>
>         *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org
>         <mailto:llvm-dev at lists.llvm.org>>, "Philip
Reames"
>         <listmail at philipreames.com
>         <mailto:listmail at philipreames.com>>, "Duncan P. N.
Exon Smith"
>         <dexonsmith at apple.com <mailto:dexonsmith at
apple.com>>,
>         "Xinliang David Li" <xinliangli at gmail.com
>         <mailto:xinliangli at gmail.com>>, "Sanjoy Das"
>         <sanjoy at playingwithpointers.com
>         <mailto:sanjoy at playingwithpointers.com>>
>
>         *Sent: *Friday, February 26, 2016 9:41:23 PM
>
>
>         *Subject: *Re: [llvm-dev] Possible soundness issue with
>         available_externally (split from "RFC: Add guard
intrinsics")
>
>         On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel <hfinkel at anl.gov
>         <mailto:hfinkel at anl.gov>> wrote:
>
>                 *From: *"Chandler Carruth" <chandlerc at
google.com
>                 <mailto:chandlerc at google.com>>
>                 *To: *"Hal Finkel" <hfinkel at anl.gov
>                 <mailto:hfinkel at anl.gov>>
>
>                 *Cc: *"llvm-dev" <llvm-dev at lists.llvm.org
>                 <mailto:llvm-dev at lists.llvm.org>>, "Philip
Reames"
>                 <listmail at philipreames.com
>                 <mailto:listmail at philipreames.com>>,
"Duncan P. N.
>                 Exon Smith" <dexonsmith at apple.com
>                 <mailto:dexonsmith at apple.com>>, "Xinliang
David Li"
>                 <xinliangli at gmail.com <mailto:xinliangli at
gmail.com>>,
>                 "Sanjoy Das" <sanjoy at
playingwithpointers.com
>                 <mailto:sanjoy at playingwithpointers.com>>
>                 *Sent: *Friday, February 26, 2016 9:33:55 PM
>
>
>                 *Subject: *Re: [llvm-dev] Possible soundness issue
>                 with available_externally (split from "RFC: Add guard
>                 intrinsics")
>
>                 On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel
>                 <hfinkel at anl.gov <mailto:hfinkel at
anl.gov>> wrote:
>
>
>                     > From: "Chandler Carruth" <chandlerc
at google.com
>                     <mailto:chandlerc at google.com>>
>                     > To: "Hal Finkel" <hfinkel at anl.gov
>                     <mailto:hfinkel at anl.gov>>, "Sanjoy
Das"
>                     <sanjoy at playingwithpointers.com
>                     <mailto:sanjoy at playingwithpointers.com>>
>                     > Cc: "llvm-dev" <llvm-dev at
lists.llvm.org
>                     <mailto:llvm-dev at lists.llvm.org>>,
"Philip Reames"
>                     <listmail at philipreames.com
>                     <mailto:listmail at philipreames.com>>,
"Duncan P. N.
>                     Exon Smith"
>                     > <dexonsmith at apple.com
>                     <mailto:dexonsmith at apple.com>>,
"Xinliang David
>                     Li" <xinliangli at gmail.com
>                     <mailto:xinliangli at gmail.com>>
>                     > Sent: Friday, February 26, 2016 9:01:48 PM
>                     > Subject: Re: [llvm-dev] Possible soundness issue
>                     with available_externally (split from "RFC: Add
>                     guard intrinsics")
>                     >
>                     >
>                     > I think this will have a much higher cost than
>                     my proposal to
>                     > constrain how we deduce function attributes
>                     (which still fixes
>                     > Sanjoy's latest example).
>                     >
>                     > Specifically, I think this will force us to
>                     constrain far too many
>                     > transformations for the sake of code size in
>                     functions that we won't
>                     > inline. Even if we were never going to deduce
>                     function attributes
>                     > for anything in the function (because its big
>                     and reads and writes
>                     > everything), we'll still have to constrain our
>                     transformations just
>                     > because we *might* later deduce a function
>                     attribute that triggers
>                     > these kinds of bugs.
>                     >
>                     > Essentially, you're proposing to limit
>                     intraprocedural optimization
>                     > to when we can successfully to interprocedural
>                     optimization
>                     > ("privatization"), where I'm
suggesting we limit
>                     interprocedural
>                     > optimization to leave intraprocedural
>                     optimization unconstrained.
>                     > Given the ratio of our optimizations (almost all
>                     are intra, very few
>                     > are inter), I'm much more comfortable with the
>                     latter.
>
>                     This is a good point; we can certainly (easily)
>                     delay the privatization decision until we modify
>                     any IPA-level function information (at which point
>                     we can either reject the attribute change (when
>                     optimizing for code size), or keep it locally
>                     (when optimizing for speed). Ideally, you'd want
>                     to delay this even further (until you knew the
>                     attribute information was used), but I'm not sure
>                     that's practical.
>
>                     Actually, what if instead of actually privatizing,
>                     we moved the function into a different comdat
>                     section named after some hash of the function
>                     body? That way, if all versions are actually
>                     optimized identically, we'll still only end up
>                     with one copy in the final executable. If this is
>                     technically possible, it seems like the best kind
>                     of solution.
>
>
>                 This is how I want to do a revamped function merging
>                 anyways and it would fall out naturally of that.
>
>             Excellent, so let's fix this at the same time we implement
>             this function merging (so that we don't have performance
>             regressions in an intermediate state). This will also
>             allow us to have uniform logic across different
>             optimization levels, which is obviously preferable.
>
>
>         I am *extremely* uncomfortable waiting to fix this until
>         merging stuff is in place and we add privatization heuristics
>         to our IPO passes. Those might be years away.
>
>     I'm *extremely* uncomfortable fixing this at all unless it can be
>     done without causing performance regressions. The underlying basic
>     use case (linking together code compiled with different
>     optimization levels), is certainly something I'd like to work
>     properly, but is definitely a far lower priority than optimized
>     code quality and size.
>
>
> Ok, I prioritize this differently: it is absolutely critical. Every 
> binary I have includes files that fit this description. Every single 
> binary.
Chandler, I'm more in agreement with Hal here.  I see your point, but 
given gcc appears to be getting this equally wrong, and we've never seen 
this case in practice, I don't see fixing this as a critical issue.  I'm
also strongly in agreement with Hal that a fix which reverts performance 
for the normal case is unacceptable unless all options have been 
explored and there really are no other choices.
>
> I think that we will discover subtle miscompiles every time we try to 
> make function attributes more powerful until this is fixed. I think 
> this is absolutely critical to fix.
Not sure this is actual true.  In particular, there have been several 
changes to our IPO passes over the last year and I don't remember 
hearing any screaming.  :)

Philip

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160229/ce4a3fa8/attachment.html>