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: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:
> > ----- Original Message -----
> 
> > > 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.

Thanks again, 
Hal 

-- 

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/f1841f42/attachment.html>

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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160227/64a12a2a/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 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>