[PATCH] Caller/Calllee unsafe-fp-math attribute fixup prior to inlining.

[Chandler Carruth]

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/20140418/0595c408/attachment.html>