[PATCH] Caller/Calllee unsafe-fp-math attribute fixup prior to inlining.
Chandler Carruth
chandlerc at google.com
Mon May 5 11:47:56 PDT 2014
+ /// @brief Return true if attribute A is set to V (defaults to "true").
+ bool isFnAttributeSet(StringRef A, StringRef V = "true") const {
+ if (!hasFnAttribute(A)) return false;
+ StringRef AV = getAttributes()
+ .getAttribute(AttributeSet::FunctionIndex, A)
+ .getValueAsString();
+
+ return AV == V;
+ }
clang-format?
+
+ /// @brief Removes unsafe attributes from this function if they are not
in the
+ /// provided callee. The purpose of this helper method is to make the
caller
+ /// as conservatively safe (for unsafe math) as the callee that is about
to be
+ /// inlined into it. This is necessary at the moment since SelectionDag
does
+ /// not support per-instruction fast-math flags.
+ void StripUnsafeAttrs(const Function *Callee);
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.
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.
The only clean way I see to do this is:
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.
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)
3) In the inliner, compute the unsafe-fp-math attributes from the callee
and intersect them with the caller attribute set.
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.
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.
On Sun, May 4, 2014 at 11:40 PM, Puyan Lotfi <plotfi at apple.com> wrote:
> Ping.
>
> PL
>
>
>
>
> On Apr 29, 2014, at 1:18 PM, Puyan Lotfi <plotfi at apple.com> wrote:
>
> Hi Chandler, sorry for the late reply:
>
> I looked over your comments, and this is what I’ve got:
>
> <unsafeattr5.patch>
>
>
> Summary:
>
> 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.
> 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.
> 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).
> 4) Changed the test case to check for the the stripped attributes, rather
> than looking for the “foo”=“false” attributes.
> 5) Also, added comments.
>
> I think that roughly sums it up, hopefully this is sufficient.
>
> Thanks
>
> -Puyan
>
> On Apr 18, 2014, at 3:59 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> Cool, this approach makes sense. My remaining comments are about how to
> structure the code.
>
> FWIW, using Phabricator should make this much easier in the future. =]
>
> +/// isAttrSet - Check if string attribute a is set to true in function F.
> +static bool isAttrSet(const Function *F, const char *a) {
> + return (F->hasFnAttribute(a)) &&
> + (F->getAttributes()
> + .getAttribute(AttributeSet::FunctionIndex, a)
> + .getValueAsString() == "true");
> +}
>
> This should be a method on Function I think. The inliner really isn't
> special here.
>
> 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.
>
> 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.
>
> +
> +/// fixupCallerAttrs - Removes unsafe attributes from caller if not in
> callee.
> +static void fixupCallerAttrs(Function *Caller, const Function *Callee) {
>
> 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).
>
> 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.
>
> + std::vector<const char*> Attrs = { "less-precise-fpmad",
> "unsafe-fp-math" };
> + for (auto I : Attrs) {
> + if ((isAttrSet(Caller, I)) && !isAttrSet(Callee, I)) {
> + AttrBuilder BR, BA;
> + LLVMContext &CC = Caller->getContext();
> + BR.addAttribute(Attribute::get(CC, I, "true" ));
> + BA.addAttribute(Attribute::get(CC, I, "false"));
>
> Why do you need to add false as opposed to just removing true? I feel like
> this should just be a stripping operation?
>
> +
> + const unsigned FI = AttributeSet::FunctionIndex;
> + Caller->setAttributes(Caller->getAttributes()
> + .removeAttributes(CC, FI, AttributeSet::get(CC, FI, BR))
> + .addAttributes(CC, FI, AttributeSet::get(CC, FI, BA)));
> + }
>
> 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?
>
>
> On Thu, Apr 17, 2014 at 10:57 AM, Puyan Lotfi <plotfi at apple.com> wrote:
>
>> I’ve attached a simplified version of the patch.
>> All it does now is check the attributes 1:1, and removes them from the
>> caller if they aren’t in the callee.
>> We can remove this when selectiondag is updated.
>>
>> -PL
>>
>>
>>
>> On Apr 16, 2014, at 1:07 AM, Puyan Lotfi <plotfi at apple.com> wrote:
>>
>>
>> On Apr 16, 2014, at 1:05 AM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> On Wed, Apr 16, 2014 at 12:58 AM, Puyan Lotfi <plotfi at apple.com> wrote:
>>
>>>
>>> On Apr 16, 2014, at 12:34 AM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>>
>>> On Wed, Apr 16, 2014 at 12:23 AM, Puyan Lotfi <plotfi at apple.com> wrote:
>>>
>>>> Hi Chandler
>>>>
>>>> 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.
>>>>
>>>
>>> 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.
>>>
>>>
>>>
>>> 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.
>>>
>>
>> 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.
>>
>>
>> Alright, cool. I’ll put together a somewhat more simplified and
>> straightforward version of this patch then.
>>
>>
>>> 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.
>>>
>>>
>>> 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.
>>>
>>> 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.
>>>
>>>
>>>> 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.
>>>>
>>>
>>> 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.
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140505/7335878b/attachment.html>
More information about the llvm-commits
mailing list