I think I should move the discussion to other command line options for now.
Based on the feedback I received, it seems that fixing selection-dag to
model fp math flags at instruction (selection-dag node) level is the
direction we should be heading. I believe that will be a fairly large
project and also it's mostly orthogonal to the problem I'm trying to solve.
I'll look into it later when I am done fixing the other options.
Before I move on to the next command line option, I'd like to discuss a few
more things that were brought up in this thread. Let me know what you think.
Hi Eric,
Hi Akira,
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?
Instead of changing the IR, I'm considering adding a flag to the fields of
TargetOptions that indicates whether the corresponding option has been
specified on the command line. Initially, changing the function attribute
value in the IR seemed to be a good idea (or at least acceptable) to me,
but changing all instructions with fp math flags in the IR is probably not
a good idea.
I'm considering defining a template class like this:
template<typename ValTy>
struct Option {
ValTy Val;
bool Occurred;
};
Option<unsigned> StackAlignmentOverride;
The field "Occurred" is used to decide whether Option.Val should override
the function attribute value in the IR. For example, passing
"-stack-alignment 64" to llc sets StackAlignmentOverride=(64, true). The
backend passes will look at StackAlignmentOverride and get its value if
Occurred==true, otherwise try to get the function attribute value in the 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.
Are there any plans to define a separate class for the module-level
subtarget? The reason I ask is that it seems pretty easy to mistakenly use
the module-level subtarget when the function-level subtarget should be used
and those kind of bugs are very hard to find.