[PATCH] [Inliner] Use whitelist instead of blacklist when checking function attribute compatibility and make the check stricter
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed Mar 4 11:27:20 PST 2015
> On 2015-Mar-03, at 17:48, Chandler Carruth <chandlerc at gmail.com> wrote:
> I really don't like this. I really don't like the prior solution either.
> Today, we have subtle, hard to diagnose correctness bugs. Bad bad bad.
> With this we have subtle, hard to diagnose performance bugs combined with a bit of a maintenance nightmare to keep this up to date. Bad bad bad.
> I also fear the performance bugs will in practice happen much more often. I'm seriously worried about the string attributes for example.
Of course performance and correctness both matter, but
correct-by-default is more principled than fast-by-default. Of the two
approaches (in-tree/blacklist vs. Akira's/whitelist), the whitelist
makes it clearer which attributes have been audited for correctness, and
it puts the burden on someone adding a new attribute to prove (or at
least make a conscience decision) that inlining across functions is
(Side-note: actually, it brings to light that no one has actually
audited this list. Many of these look incorrect to inline across.)
It's particularly important to have inlining off-by-default for CodeGen
attributes (particularly target-specific attributes); these will end up
as string attributes.
Why are you worried about string attributes? Here's the current list:
The first five are all floating-point math related, and IIUC it's a
correctness bug to inline across those. The rest are CodeGen-specific
attributes. IMO, there should be a fairly high burden of proof that
inlining across functions with different CodeGen attributes is correct.
> I think we need to back up and think of a better design for this. A *much* better design. Do you have any thoughts on how to do this in a more maintainable way? If not I'll try to write something else.
I don't see what's particularly unmaintainable about this.
> Naturally, if there are specific attributes that are casuing problems, let's get a targeted fix in place for those attributes.
More information about the llvm-commits