<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 12, 2015 at 2:26 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:</div><div class="gmail_quote"><br></div><div class="gmail_quote">Hi Eric,</div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Akira,<br><br><div class="gmail_quote"><span>On Thu Jan 08 2015 at 5:36:54 PM Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>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.</div><div><br></div><div>Here is the link to the original discussion I started last year:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078785.html</a><br></div><div><br></div></div></blockquote><div><br></div></span><div>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.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div>These are the changes I plan to make:<br></div><div><p>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).</p><p>2. Replace usages of TargetOptions::UnsafeFPMath with calls to a<span style="font-size:13.1999998092651px"> </span></p></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><p>function which gets the value of attribute “unsafe-fp-math” in the IR.</p></div></div></blockquote><div><br></div></span><div>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?</div><span><div> </div></span></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><p>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).</p><p></p></div></div></blockquote><div><br></div></span><div>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.</div><span><div> </div></span></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><p>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.</p></div></div></blockquote><div><br></div></span><div>This can be done first if you like at the moment, though you may run into other problems and testing may be somewhat minimal.</div><span><div></div></span></div></blockquote><div><br></div><div>OK. I'll spend a little more time investigating and testing and send a patch for it if everything goes well.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><p>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).</p><p><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-December/079904.html</a><br></p><p></p></div></div></blockquote></span><div>Yes.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><p>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.</p></div></blockquote><div><br></div></span><div>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.</div><div><br></div><div>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.</div><span><font color="#888888"><div><br></div></font></span></div></blockquote><div><br></div><div>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?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><span><font color="#888888"><div></div><div>-eric </div></font></span></div>
</blockquote></div><br></div></div>