[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
legal.

(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:

  - less-precise-fpmad
  - no-infs-fp-math
  - no-nans-fp-math
  - unsafe-fp-math
  - use-soft-float
  - no-frame-pointer-elim
  - no-frame-pointer-elim-non-leaf
  - no-realign-stack
  - split-stack
  - stack-protector-buffer-size

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 mailing list