llvm dev - Jan 2015 - [LLVMdev] Enable changing UnsafeFPMath on a per-function basis

To continue the discussion I started last year (see the link below) on
embedding command-line options in bitcode, I came up with a plan to improve
the way the backend changes UnsafeFPMath on a per-function basis. The code
in trunk currently resets TargetOptions::UnsafeFPMath at the beginning of
SelectionDAGISel::runOnMachineFunction to enable compiling one function
with “unsafe-fp-math=true” and another with “unsafe-fp-math=false”, but
this won’t work if we want to parallelize the backend in the future, which
is something Eric mentioned in his talk last year.

Here is the link to the original discussion I started last year:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html

These are the changes I plan to make:

1. In llc.cpp (and any other tools that use the option), change the
function attribute in the IR based on the value of command line option
“enable-unsafe-pf-math” (EnableUnsafeFPMath, defined in CommandFlags.h).

2. Replace usages of TargetOptions::UnsafeFPMath with calls to a function
which gets the value of attribute “unsafe-fp-math” in the IR.

3. Stringify function attribute “unsafe-fp-math” and append it to the
string that is used as the lookup key in
TargetMachine::getSubtargetImpl(const Function&). Also, pass the function
attribute to the subtarget constructors that need it to construct itself
(e.g., ARM) or construct one of the backend objects (e.g., X86, which needs
it in the constructor of X86TargetLowering).

4. In MachineFunction’s constructor, call
TargetMachine::getSubtargetImpl(const Function &) to initialize STI
(pointer to the per-function subtarget object). Currently, the version of
TargetMachine::getSubtargetImpl that doesn’t take a const Function&
parameter is called, which returns the module-level subtarget object.

5.  Fix ARMAsmPrinter::emitAttributes to compute the value of
TargetOptions::UnsafeFPMath based on the function attributes of all the
functions in the module being compiled (see the link below).

http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html

6. Move the code in CGCall.cpp that sets the function attributes to
BackendUtil.cpp. clang should set the function attributes regardless of
whether it is compiling from source code or from an IR file (e.g., clang
foo1.ll -o foo1.s -ffast-math), but currently this happens only if it’s
compiling from source.

Any comments and suggestions are welcome. If we can agree on the main
issues, I'll follow up with patches that implement the changes I proposed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150108/2bf9e597/attachment.html>

Whatever happened to tracking the safe-or-fast-ness of FP math on
instructions? Is tracking this property at a function granularity correct?
I seem to recall nobody wanted to thread this through the SDAG.

If we agree that we want to track this at the function level, then your
proposal seems reasonable.


On Thu, Jan 8, 2015 at 5:36 PM, Akira Hatanaka <ahatanak at gmail.com>
wrote:
> To continue the discussion I started last year (see the link below) on
> embedding command-line options in bitcode, I came up with a plan to improve
> the way the backend changes UnsafeFPMath on a per-function basis. The code
> in trunk currently resets TargetOptions::UnsafeFPMath at the beginning of
> SelectionDAGISel::runOnMachineFunction to enable compiling one function
> with “unsafe-fp-math=true” and another with “unsafe-fp-math=false”, but
> this won’t work if we want to parallelize the backend in the future, which
> is something Eric mentioned in his talk last year.
>
> Here is the link to the original discussion I started last year:
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html
>
> These are the changes I plan to make:
>
> 1. In llc.cpp (and any other tools that use the option), change the
> function attribute in the IR based on the value of command line option
> “enable-unsafe-pf-math” (EnableUnsafeFPMath, defined in CommandFlags.h).
>
> 2. Replace usages of TargetOptions::UnsafeFPMath with calls to a function
> which gets the value of attribute “unsafe-fp-math” in the IR.
>
> 3. Stringify function attribute “unsafe-fp-math” and append it to the
> string that is used as the lookup key in
> TargetMachine::getSubtargetImpl(const Function&). Also, pass the
function
> attribute to the subtarget constructors that need it to construct itself
> (e.g., ARM) or construct one of the backend objects (e.g., X86, which needs
> it in the constructor of X86TargetLowering).
>
> 4. In MachineFunction’s constructor, call
> TargetMachine::getSubtargetImpl(const Function &) to initialize STI
> (pointer to the per-function subtarget object). Currently, the version of
> TargetMachine::getSubtargetImpl that doesn’t take a const Function&
> parameter is called, which returns the module-level subtarget object.
>
> 5.  Fix ARMAsmPrinter::emitAttributes to compute the value of
> TargetOptions::UnsafeFPMath based on the function attributes of all the
> functions in the module being compiled (see the link below).
>
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html
>
> 6. Move the code in CGCall.cpp that sets the function attributes to
> BackendUtil.cpp. clang should set the function attributes regardless of
> whether it is compiling from source code or from an IR file (e.g., clang
> foo1.ll -o foo1.s -ffast-math), but currently this happens only if it’s
> compiling from source.
>
> Any comments and suggestions are welcome. If we can agree on the main
> issues, I'll follow up with patches that implement the changes I
proposed.
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150112/83d17472/attachment.html>

----- Original Message -----
> From: "Reid Kleckner" <rnk at google.com>
> To: "Akira Hatanaka" <ahatanak at gmail.com>
> Cc: "LLVM Developers Mailing List" <llvmdev at cs.uiuc.edu>
> Sent: Monday, January 12, 2015 2:54:48 PM
> Subject: Re: [LLVMdev] Enable changing UnsafeFPMath on a per-function basis
> 
> 
> 
> Whatever happened to tracking the safe-or-fast-ness of FP math on
> instructions? Is tracking this property at a function granularity
> correct? I seem to recall nobody wanted to thread this through the
> SDAG.
No, I think we did want to do that, just no one has yet done it. We now have
NSW/NUW in SDAG, so it should not be too much different for the FP flags.

 -Hal
> 
> 
> If we agree that we want to track this at the function level, then
> your proposal seems reasonable.
> 
> 
> 
> 
> 
> 
> 
> On Thu, Jan 8, 2015 at 5:36 PM, Akira Hatanaka < ahatanak at gmail.com
>
> wrote:
> 
> 
> 
> 
> To continue the discussion I started last year (see the link below)
> on embedding command-line options in bitcode, I came up with a plan
> to improve the way the backend changes UnsafeFPMath on a
> per-function basis. The code in trunk currently resets
> TargetOptions::UnsafeFPMath at the beginning of
> SelectionDAGISel::runOnMachineFunction to enable compiling one
> function with “unsafe-fp-math=true” and another with
> “unsafe-fp-math=false”, but this won’t work if we want to
> parallelize the backend in the future, which is something Eric
> mentioned in his talk last year.
> 
> 
> Here is the link to the original discussion I started last year:
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html
> 
> 
> 
> These are the changes I plan to make:
> 
> 
> 
> 1. In llc.cpp (and any other tools that use the option), change the
> function attribute in the IR based on the value of command line
> option “enable-unsafe-pf-math” (EnableUnsafeFPMath, defined in
> CommandFlags.h).
> 
> 2. Replace usages of TargetOptions::UnsafeFPMath with calls to a
> function which gets the value of attribute “unsafe-fp-math” in the
> IR.
> 
> 3. Stringify function attribute “unsafe-fp-math” and append it to the
> string that is used as the lookup key in
> TargetMachine::getSubtargetImpl(const Function&). Also, pass the
> function attribute to the subtarget constructors that need it to
> construct itself (e.g., ARM) or construct one of the backend objects
> (e.g., X86, which needs it in the constructor of X86TargetLowering).
> 
> 4. In MachineFunction’s constructor, call
> TargetMachine::getSubtargetImpl(const Function &) to initialize STI
> (pointer to the per-function subtarget object). Currently, the
> version of TargetMachine::getSubtargetImpl that doesn’t take a const
> Function& parameter is called, which returns the module-level
> subtarget object.
> 
> 5. Fix ARMAsmPrinter::emitAttributes to compute the value of
> TargetOptions::UnsafeFPMath based on the function attributes of all
> the functions in the module being compiled (see the link below).
> 
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html
> 
> 
> 6. Move the code in CGCall.cpp that sets the function attributes to
> BackendUtil.cpp. clang should set the function attributes regardless
> of whether it is compiling from source code or from an IR file
> (e.g., clang foo1.ll -o foo1.s -ffast-math), but currently this
> happens only if it’s compiling from source.
> 
> Any comments and suggestions are welcome. If we can agree on the main
> issues, I'll follow up with patches that implement the changes I
> proposed.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 
-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory

Hi Akira,

On Thu Jan 08 2015 at 5:36:54 PM Akira Hatanaka <ahatanak at gmail.com>
wrote:
> To continue the discussion I started last year (see the link below) on
> embedding command-line options in bitcode, I came up with a plan to improve
> the way the backend changes UnsafeFPMath on a per-function basis. The code
> in trunk currently resets TargetOptions::UnsafeFPMath at the beginning of
> SelectionDAGISel::runOnMachineFunction to enable compiling one function
> with “unsafe-fp-math=true” and another with “unsafe-fp-math=false”, but
> this won’t work if we want to parallelize the backend in the future, which
> is something Eric mentioned in his talk last year.
>
> Here is the link to the original discussion I started last year:
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html
>
>
Thanks for coming back around to this. I'd hoped to have most of this
handled, but as you can tell it's a nice herd of yaks to get everything
playing nicely.

> These are the changes I plan to make:
>
> 1. In llc.cpp (and any other tools that use the option), change the
> function attribute in the IR based on the value of command line option
> “enable-unsafe-pf-math” (EnableUnsafeFPMath, defined in CommandFlags.h).
>
> 2. Replace usages of TargetOptions::UnsafeFPMath with calls to a
>
function which gets the value of attribute “unsafe-fp-math” in the
IR.
>
These two combined seems a little weird. Do you mean that you plan on,
effectively, changing the IR if someone passes a command line flag to an
existing set of IR?

> 3. Stringify function attribute “unsafe-fp-math” and append it to the
> string that is used as the lookup key in
> TargetMachine::getSubtargetImpl(const Function&). Also, pass the
function
> attribute to the subtarget constructors that need it to construct itself
> (e.g., ARM) or construct one of the backend objects (e.g., X86, which needs
> it in the constructor of X86TargetLowering).
>
>
Another thought is to put it as part of the feature string rather than
adding it as a separate attribute. That way you won't have to add a lookup
in each target.

> 4. In MachineFunction’s constructor, call
> TargetMachine::getSubtargetImpl(const Function &) to initialize STI
> (pointer to the per-function subtarget object). Currently, the version of
> TargetMachine::getSubtargetImpl that doesn’t take a const Function&
> parameter is called, which returns the module-level subtarget object.
>
This can be done first if you like at the moment, though you may run into
other problems and testing may be somewhat minimal.

> 5.  Fix ARMAsmPrinter::emitAttributes to compute the value of
> TargetOptions::UnsafeFPMath based on the function attributes of all the
> functions in the module being compiled (see the link below).
>
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html
>
> Yes.
> 6. Move the code in CGCall.cpp that sets the function attributes to
> BackendUtil.cpp. clang should set the function attributes regardless of
> whether it is compiling from source code or from an IR file (e.g., clang
> foo1.ll -o foo1.s -ffast-math), but currently this happens only if it’s
> compiling from source.
>
This is an interesting solution, but involves changing IR files and I don't
know that we want to do this (as I mentioned above). But I do admit I like
it more than having to check both TargetOptions and the attribute to do it.

Another thing you haven't brought up is the inliner (or other cross
function optimizations)- what do you plan on doing there? Think two files,
one compiled with and one compiled without and linked together.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150112/513df75e/attachment.html>

Basically my plan is to change the backend to use the value of
"unsafe-fp-math" that is recorded in the IR as a function attribute
instead
of reading the value from TargetOptions::UnsafeFPMath (which is global). It
doesn't change at which level (function or instruction level) fast FP math
is tracked or make it harder to change selection sag to track it at a finer
granularity, if someone decides to do that later.

On Mon, Jan 12, 2015 at 12:54 PM, Reid Kleckner <rnk at google.com> wrote:
> Whatever happened to tracking the safe-or-fast-ness of FP math on
> instructions? Is tracking this property at a function granularity correct?
> I seem to recall nobody wanted to thread this through the SDAG.
>
> If we agree that we want to track this at the function level, then your
> proposal seems reasonable.
>
>
> On Thu, Jan 8, 2015 at 5:36 PM, Akira Hatanaka <ahatanak at
gmail.com> wrote:
>
>> To continue the discussion I started last year (see the link below) on
>> embedding command-line options in bitcode, I came up with a plan to
improve
>> the way the backend changes UnsafeFPMath on a per-function basis. The
code
>> in trunk currently resets TargetOptions::UnsafeFPMath at the beginning
of
>> SelectionDAGISel::runOnMachineFunction to enable compiling one function
>> with “unsafe-fp-math=true” and another with “unsafe-fp-math=false”, but
>> this won’t work if we want to parallelize the backend in the future,
which
>> is something Eric mentioned in his talk last year.
>>
>> Here is the link to the original discussion I started last year:
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html
>>
>> These are the changes I plan to make:
>>
>> 1. In llc.cpp (and any other tools that use the option), change the
>> function attribute in the IR based on the value of command line option
>> “enable-unsafe-pf-math” (EnableUnsafeFPMath, defined in
CommandFlags.h).
>>
>> 2. Replace usages of TargetOptions::UnsafeFPMath with calls to a
function
>> which gets the value of attribute “unsafe-fp-math” in the IR.
>>
>> 3. Stringify function attribute “unsafe-fp-math” and append it to the
>> string that is used as the lookup key in
>> TargetMachine::getSubtargetImpl(const Function&). Also, pass the
function
>> attribute to the subtarget constructors that need it to construct
itself
>> (e.g., ARM) or construct one of the backend objects (e.g., X86, which
needs
>> it in the constructor of X86TargetLowering).
>>
>> 4. In MachineFunction’s constructor, call
>> TargetMachine::getSubtargetImpl(const Function &) to initialize STI
>> (pointer to the per-function subtarget object). Currently, the version
of
>> TargetMachine::getSubtargetImpl that doesn’t take a const Function&
>> parameter is called, which returns the module-level subtarget object.
>>
>> 5.  Fix ARMAsmPrinter::emitAttributes to compute the value of
>> TargetOptions::UnsafeFPMath based on the function attributes of all the
>> functions in the module being compiled (see the link below).
>>
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html
>>
>> 6. Move the code in CGCall.cpp that sets the function attributes to
>> BackendUtil.cpp. clang should set the function attributes regardless of
>> whether it is compiling from source code or from an IR file (e.g.,
clang
>> foo1.ll -o foo1.s -ffast-math), but currently this happens only if it’s
>> compiling from source.
>>
>> Any comments and suggestions are welcome. If we can agree on the main
>> issues, I'll follow up with patches that implement the changes I
proposed.
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150112/db9946bd/attachment.html>

On Mon, Jan 12, 2015 at 2:26 PM, Eric Christopher <echristo at gmail.com>
wrote:

Hi Eric,

Hi Akira,
>
> On Thu Jan 08 2015 at 5:36:54 PM Akira Hatanaka <ahatanak at
gmail.com>
> wrote:
>
>> To continue the discussion I started last year (see the link below) on
>> embedding command-line options in bitcode, I came up with a plan to
improve
>> the way the backend changes UnsafeFPMath on a per-function basis. The
code
>> in trunk currently resets TargetOptions::UnsafeFPMath at the beginning
of
>> SelectionDAGISel::runOnMachineFunction to enable compiling one function
>> with “unsafe-fp-math=true” and another with “unsafe-fp-math=false”, but
>> this won’t work if we want to parallelize the backend in the future,
which
>> is something Eric mentioned in his talk last year.
>>
>> Here is the link to the original discussion I started last year:
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html
>>
>>
> Thanks for coming back around to this. I'd hoped to have most of this
> handled, but as you can tell it's a nice herd of yaks to get everything
> playing nicely.
>
>
>> These are the changes I plan to make:
>>
>> 1. In llc.cpp (and any other tools that use the option), change the
>> function attribute in the IR based on the value of command line option
>> “enable-unsafe-pf-math” (EnableUnsafeFPMath, defined in
CommandFlags.h).
>>
>> 2. Replace usages of TargetOptions::UnsafeFPMath with calls to a
>>
> function which gets the value of attribute “unsafe-fp-math” in the IR.
>>
>
> These two combined seems a little weird. Do you mean that you plan on,
> effectively, changing the IR if someone passes a command line flag to an
> existing set of IR?
>
>
Yes, my current plan is to change the existing attributes in the IR when
command line options are passed. I wasn't sure if changing the IR was a
good idea, but it seemed to be the simplest way to handle command line
options passed to llc using the current cl::opt infrastructure. This also
depends on how the command line infrastructure will look like after Chris
checks in his patches.

3. Stringify function attribute “unsafe-fp-math” and append it to
the
>> string that is used as the lookup key in
>> TargetMachine::getSubtargetImpl(const Function&). Also, pass the
function
>> attribute to the subtarget constructors that need it to construct
itself
>> (e.g., ARM) or construct one of the backend objects (e.g., X86, which
needs
>> it in the constructor of X86TargetLowering).
>>
>>
> Another thought is to put it as part of the feature string rather than
> adding it as a separate attribute. That way you won't have to add a
lookup
> in each target.
>
>
Yes, that's another way to pass the option, which is probably less
error-prone than passing each function attribute. Does it mean that we
define SubtargetFeatures for unsafe-fp-math and other options too? The
downside of this approach is that it will needlessly create new subtarget
objects for targets that currently don't need to specialize subtargets
based on the value of "unsafe-fp-math", although I'm not sure how
much
impact it will have on memory.

4. In MachineFunction’s constructor, call
>> TargetMachine::getSubtargetImpl(const Function &) to initialize STI
>> (pointer to the per-function subtarget object). Currently, the version
of
>> TargetMachine::getSubtargetImpl that doesn’t take a const Function&
>> parameter is called, which returns the module-level subtarget object.
>>
>
> This can be done first if you like at the moment, though you may run into
> other problems and testing may be somewhat minimal.
>
OK. I'll spend a little more time investigating and testing and send a
patch for it if everything goes well.

>
>
>> 5.  Fix ARMAsmPrinter::emitAttributes to compute the value of
>> TargetOptions::UnsafeFPMath based on the function attributes of all the
>> functions in the module being compiled (see the link below).
>>
>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html
>>
>> Yes.
>
>
>> 6. Move the code in CGCall.cpp that sets the function attributes to
>> BackendUtil.cpp. clang should set the function attributes regardless of
>> whether it is compiling from source code or from an IR file (e.g.,
clang
>> foo1.ll -o foo1.s -ffast-math), but currently this happens only if it’s
>> compiling from source.
>>
>
> This is an interesting solution, but involves changing IR files and I
> don't know that we want to do this (as I mentioned above). But I do
admit I
> like it more than having to check both TargetOptions and the attribute to
> do it.
>
> Another thing you haven't brought up is the inliner (or other cross
> function optimizations)- what do you plan on doing there? Think two files,
> one compiled with and one compiled without and linked together.
>
>
I haven't thought much about how I should fix the inliner as I was thinking
this should be tackled as a separate problem. Since "unsafe-fp-math"
is
already embedded in the IR as a function attribute, we don't have to wait
for my changes to be checked in and we can start fixing the problem today.
Is that right?

> -eric
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150113/3e8f5f1e/attachment.html>