<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 5, 2014, at 11:47 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div>+ /// @brief Return true if attribute A is set to V (defaults to "true").</div><div>+ bool isFnAttributeSet(StringRef A, StringRef V = "true") const {</div><div>+ if (!hasFnAttribute(A)) return false;</div>
<div>+ StringRef AV = getAttributes()</div><div>+ .getAttribute(AttributeSet::FunctionIndex, A)</div><div>+ .getValueAsString();</div><div>+</div><div>+ return AV == V;</div><div>+ }</div><div><br></div>
<div>
clang-format?</div><div><br></div></div></blockquote><div><br></div><div>Sure, I can fix the formatting here.</div><br><blockquote type="cite"><div dir="ltr"><div>+</div><div>+ /// @brief Removes unsafe attributes from this function if they are not in the</div><div>+ /// provided callee. The purpose of this helper method is to make the caller</div>
<div>+ /// as conservatively safe (for unsafe math) as the callee that is about to be</div><div>+ /// inlined into it. This is necessary at the moment since SelectionDag does</div><div>+ /// not support per-instruction fast-math flags.</div>
<div>+ void StripUnsafeAttrs(const Function *Callee);</div><div><br></div><div>This is not really what I had in mind. You've sunk *all* the logic down, which doesn't make sense as this use case is very particular to the inliner. I was suggesting to just expose part of the functionality in the Function API.</div>
<div><br></div></div></blockquote><div><br></div><div>I interpreted your previous comments that you preferred the strip logic to live on a method in Function. I can move them to a separate util location too. But exactly where would you rather see this logic? Could you be more specific? </div><br><blockquote type="cite"><div dir="ltr"><div>To try to help move things along, I've taken a careful look at all of the attribute APIs to try to suggest *exactly* how to do this, and I've come away with a very sour taste. There really is no good way to model what you're trying to do here. I feel like we're missing a substantial amount of infrastructure needed for these types of operations, and it feels really weird to build it for a use case that is going away.</div>
<div><br></div><div>The only clean way I see to do this is:</div><div><br></div><div>1) Rather than just adding a generic 'isFnAttributeSet' method, I would add a "getUnsafeFPMathAttributeSet" method which returns an AttributeSet containing only the unsafe-fp-math attributes which happen to be set on the function.</div>
<div><br></div><div>2) Introduce the ability to intersect two attribute sets to the AttributeSet class. (this could be a separate patch, it seems generically useful) (this may also require adding some generic support for text attributes whose value is 'true'... this style of attribute seems very poorly supported as-is and so its awkward to do anything with them)</div>
<div><br></div><div>3) In the inliner, compute the unsafe-fp-math attributes from the callee and intersect them with the caller attribute set.</div><div><br></div></div></blockquote><div><br></div><div>This was very similar to what I was doing in the previous patch to this one, I just happened to contain everything in one helper inside the inliner so that it would be contained and surrounded by comments specifying that it needs to be refactored in the future.</div><br><blockquote type="cite"><div dir="ltr"><div><br></div><div>I'm really questioning whether this is worth it at all. These all feel like terrible hacks around a design that just isn't going to work. If we're going to do this at all, I think we should first invest the time to make the unsafe-fp-math attributes first-class attributes. But we don't think that such attributes are the right design, so I don't know why we're investing so much effort into LLVM to hack at them and shove things that are the *wrong design* together to kinda sorta work. Instead, I think we should actually auto-upgrade away these attributes so that they are never seen in the IR at all. The replacement can be per-instruction flags. If we miss some optimizations as a consequence, I'm OK with that until support arrives. If there is some reason why we *must* persist the wrong design, I think we need to spend the time to really flesh out support for these attributes as function attributes. Give them enums, write helper functions, get reasonable intersection semantics, etc.</div>
<div><br></div></div></blockquote><div><br></div><div>We’ve already discussed that this is a stop-gap solution until we get around to adding support into SelectionDAG for per-instruction fast-math flags, and I think we need to be practical in this case until that refactoring happens. I also don’t think there was much effort that went into my initial unsafe-fp-math attribute strip helper either, it was just a small helper with comments around it saying it should be removed at some later date. </div><br><blockquote type="cite"><div dir="ltr"><div>Increasingly, I think we should simply refuse to inline function A into function B if A and B have different string attribute sets because we don't know anything about the semantics there.</div><div>
<br></div></div></blockquote><div><br></div><div>That is not an option unfortunately, we have use cases where inlining is required. </div><div><br></div><div><br></div><div><br></div><div>If you’d like, I could add the getUnsafeFPMathAttributeSet() methods on to Function and then do the intersect / strip logic as a helper to be called by the inline (in this case, where would you want the helper to live?).</div><div><br></div><div>-PL</div><br><blockquote type="cite"><div dir="ltr"><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, May 4, 2014 at 11:40 PM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Ping. </div><span class="HOEnZb"><font color="#888888"><div><br></div><div>PL</div>
<div><br></div><div></div></font></span></div>
<br><div style="word-wrap:break-word"><div></div><div><br></div><br><div><div>On Apr 29, 2014, at 1:18 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>> wrote:</div><br><blockquote type="cite">
<div style="word-wrap:break-word">Hi Chandler, sorry for the late reply:<div><br></div><div>I looked over your comments, and this is what I’ve got:</div><div><br></div><div></div></div><span><unsafeattr5.patch></span><div style="word-wrap:break-word">
<br><div><div><br></div><div>Summary:</div><div><br></div><div>1) I moved the isAttrSet function into include/llvm/IR/Function.h and renamed the method to isFnAttributeSet while restructuring the code to be a little more readable. Also, changed string types to StringRef.</div>
<div>2) Moved the strip function to a method on Function.h (proto) and Function.cpp (I could not find a good Utils file to put it in that made sense for functions, and it sounded like you preferred a method anyways). Renamed it as well.</div>
<div>3) I made it so that the stripped attributes are not re-added as false, but just removed (and I tried to re-set the attribute only once, but I had to reassign every loop iteration because of the immutable nature of attributes). </div>
<div>4) Changed the test case to check for the the stripped attributes, rather than looking for the “foo”=“false” attributes.</div><div>5) Also, added comments. </div><div><br></div><div>I think that roughly sums it up, hopefully this is sufficient. </div>
<div><br></div><div>Thanks</div><div><br></div><div>-Puyan</div><div><br><div><div>On Apr 18, 2014, at 3:59 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">Cool, this approach makes sense. My remaining comments are about how to structure the code.<div><br></div><div>FWIW, using Phabricator should make this much easier in the future. =]</div>
<div><br></div><div>
<div>+/// isAttrSet - Check if string attribute a is set to true in function F.</div><div>+static bool isAttrSet(const Function *F, const char *a) {</div><div>+ return (F->hasFnAttribute(a)) &&</div><div>+ (F->getAttributes()</div>
<div>+ .getAttribute(AttributeSet::FunctionIndex, a)</div><div>+ .getValueAsString() == "true");</div><div>+}</div><div><br></div><div>This should be a method on Function I think. The inliner really isn't special here.</div>
<div><br></div><div>Also, it should use StringRef or Twine, not a 'const char *'. I also would find the code easier to read with early return and local variables rather than being forced into a single expression.</div>
<div><br></div><div>Finally, I think you should add a better comment essentially explaining that this checks for string attributes which follow a particular pattern of the value being "true", etc.</div><div><br>
</div><div>+</div><div>+/// fixupCallerAttrs - Removes unsafe attributes from caller if not in callee.</div><div>+static void fixupCallerAttrs(Function *Caller, const Function *Callee) {</div><div><br></div><div>I would give this function a name that reflects what it does: strip a set of unsafe floating point math string attributes from a function. You can even mention that these are essentially legacy function attributes that are in the process of being phased out in favor of the instruction flags (eventually).</div>
<div><br></div><div>In that form, I again think this belongs either in the local / function transform utilities as a free function or a method on Function. I think I lean toward the latter, but if Owen or others feel differently, cool.</div>
<div><br></div><div>+ std::vector<const char*> Attrs = { "less-precise-fpmad", "unsafe-fp-math" };</div><div>+ for (auto I : Attrs) {</div><div>+ if ((isAttrSet(Caller, I)) && !isAttrSet(Callee, I)) {</div>
<div>+ AttrBuilder BR, BA;</div><div>+ LLVMContext &CC = Caller->getContext();</div><div>+ BR.addAttribute(Attribute::get(CC, I, "true" ));</div><div>+ BA.addAttribute(Attribute::get(CC, I, "false"));</div>
<div><br></div><div>Why do you need to add false as opposed to just removing true? I feel like this should just be a stripping operation?</div><div><br></div><div>+</div><div>+ const unsigned FI = AttributeSet::FunctionIndex;</div>
<div>+ Caller->setAttributes(Caller->getAttributes()</div><div>+ .removeAttributes(CC, FI, AttributeSet::get(CC, FI, BR))</div><div>+ .addAttributes(CC, FI, AttributeSet::get(CC, FI, BA)));</div>
<div>+ }</div><div><br></div><div>This seems kind of wasteful. Why not get the attribute set for the caller once, manipulate them for each unsafe math attr string, and then apply them back to the caller?</div></div></div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Apr 17, 2014 at 10:57 AM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>I’ve attached a simplified version of the patch.</div><div>All it does now is check the attributes 1:1, and removes them from the caller if they aren’t in the callee.</div><div>We can remove this when selectiondag is updated. </div>
<span><font color="#888888"><div><br></div><div>-PL</div><div><br></div></font></span></div><br><div style="word-wrap:break-word"><br><div><div>On Apr 16, 2014, at 1:07 AM, Puyan Lotfi <<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div><br>On Apr 16, 2014, at 1:05 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">On Wed, Apr 16, 2014 at 12:58 AM, Puyan Lotfi<span> </span><span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span><span> </span>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 style="word-wrap:break-word"><br><div><div><div>On Apr 16, 2014, at 12:34 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 16, 2014 at 12:23 AM, Puyan Lotfi<span> </span><span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span><span> </span>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>Hi Chandler</div><div><br></div><div>I agree on #1 (per-instruction fast-math flags) being the ideal way to do unsafe math, but it would require we make non-trivial modifications to selectiondag in order to handle them on a per instruction granularity rather than as a function attribute. The purpose of the patch is to fixup the function attributes so that the backend doesn’t non-conservativly modify what should be safe code, given the current constraints.</div>
</blockquote><div><br></div><div>I just don't think we should add hacks to the inliner rather than doing the right thing here. The right thing is to lower these to instruction flags and strip the function attributes returning the functions to conservatively correct semantics. Once you do that, you'll just get performance problems when selection dag isn't aware of them, but no correctness bugs. Then we can incrementally fix selection dag to do the right thing with the per-instruction flags.</div>
<div> </div></div></div></div></blockquote><div><br></div></div><div>I believe clang already sets the instruction flags, and all this patch does is strip the function attributes to return the behavior to conservatively correct semantics. I can simplify the way this is done so that unequal attributes are stripped 1:1 rather than adding complicated rules.</div>
</div></div></blockquote><div><br></div><div>That seems better. If the function attributes are still significant (despite Clang setting instruction flags) I feel like we should do something to properly canonicalize better within LLVM itself in addition to making sure that we at least don't violate the semantic contract of the function attributes.</div>
<div><br></div></div></div></div></blockquote><div><br></div><div>Alright, cool. I’ll put together a somewhat more simplified and straightforward version of this patch then. </div><br><blockquote type="cite"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><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 style="word-wrap:break-word">
<div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
It would be easier to bail out of inlining and take the #2 route, but we have use-cases that actually require always_inline as well.</blockquote><div><br></div><div>always_inline already has the constraint that it can only be used on functions which the optimizer happens to be able to inline in every case. For example, you can't effectively use it in certain circumstances with dynamic allocas. But the point of #2 wasn't to pick some *specific* solution of bailing on inlining. Instead, we could have the inliner always strip any fp-math-safety function attributes from the caller when inlining code into it from a callee with different fp-math-safety attributes. That's always conservatively correct and doesn't interfere with inilining.</div>
<div><br></div><div>I don't expect that to be any more useful in *practice* or the real world use cases. All either of these strategies do is provide a conservatively correct and simple model for handling the function attributes if for some reason they show up. They both *completely* rely on expressing fp math safety constraints per-instruction rather than per-function to address real-world use cases. Sorry if that wasn't clear.</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><br></div><div>I think the ideal solution would be to refactor selectiondag in the backend to not use these function attributes and to handle per-instruction fast-math flags instead, but that would take major surgery to selectiondag.</div>
</blockquote></div><br>Adding hacks to the inliner to do special things to string-named function attributes when we have the *correct* design already well understood and in place in the IR doesn't make sense to me. Any missed optimizations we have when using the correct design in the IR due to selection dag weaknesses are already problems and really need to be fixed.</div>
</div></blockquote></div></div></blockquote></div><br></div></div></blockquote></div><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">_______________________________________________</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">llvm-commits mailing list</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<a href="mailto:llvm-commits@cs.uiuc.edu" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank">llvm-commits@cs.uiuc.edu</a><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote>
</div><br></div><br></blockquote></div><br></div>
</blockquote></div><br></div></div></div></blockquote></div><br></div><br></blockquote></div><br></div>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></body></html>