Page MenuHomePhabricator

Add inherited attributes before parsed attributes.
ClosedPublic

Authored by Meinersbur on Aug 2 2018, 7:23 PM.

Details

Summary

Currently, attributes from previous declarations ('inherited attributes') are added to the end of a declaration's list of attributes. Before r338800, the attribute list was in reverse. r338800 changed the order of non-inherited (parsed from the current declaration) attributes, but inherited attributes are still appended to the end of the list.

This patch appends inherited attributes after other inherited attributes, but before any non-inherited attribute. This is to make the order of attributes in the AST correspond to the order in the source code.

Diff Detail

Event Timeline

Meinersbur created this revision.Aug 2 2018, 7:23 PM

IMO, I think this (and the next 2 numerically), show to me that we have too much dependence on the order of attributes, which all of these show is not a reliable thing to count on. I quite dislike this as well, and think this is going to lead us to chasing these same issues forever.

I wonder if the solution here is to just sort the attributes by source location? This would create that guarantee, which previously didn't exist.

For this patch the goal is to have the attributes in the AST in an order that is less surprising to consumers (including out-of-tree). If we change it now, new/revised code/diagnostics will be written to match this order.

I agree that ideally, most attributes (exceptions are e.g. enable_if) should be more like a set than a list, but I don't think we can get rid of the 'ordered-ness' in general.

I though about ordering by SourceLocation as well and could be implemented like this:
Add all SLoc's sequences for two SourceLocation to lists. Start comparing from the bottom (=> the translation units) until there is a difference.

However, I dislike the idea that the result could potentially depend on SourceLocation that previously was only used when emitting diagnostics.

test/Misc/ast-dump-attr.cpp
228–230

Note that before rC338800, these were mixed-up (not even the reverse order)

test/Sema/attr-availability-ios.c
10

Note that the deprecated marker moved to the line that actually declares the function as deprecated.

aaron.ballman edited reviewers, added: rsmith; removed: nicholas, dlj, echristo.Aug 12 2018, 8:26 AM
aaron.ballman added subscribers: rsmith, echristo, dlj, nicholas.

In general, I think this is the right way to go. I've added @rsmith to the reviewers because I'm curious what his thoughts are on this approach, though.

lib/AST/DeclBase.cpp
854–859

The unfortunate part about this is that inherited attributes are fairly common, so this extra searching may happen more often than we'd like. I wonder how bad it would be to keep track of the location within the list where the inherited attributes start. Then again, the list of attributes should be relatively short in most cases, so this search isn't likely to be too expensive.

Having some performance measurements might help decide this, too.

Meinersbur added inline comments.Aug 13 2018, 2:11 PM
lib/AST/DeclBase.cpp
854–859

Timed clang-compilation using perf (perf stat bin/clang llvm/unittests/IR/InstructionsTest.cpp ...), before this patch (r339574):

   7647.415800      task-clock (msec)         #    0.983 CPUs utilized
           289      context-switches          #    0.038 K/sec
            26      cpu-migrations            #    0.003 K/sec
        86,494      page-faults               #    0.011 M/sec
19,068,741,863      cycles                    #    2.493 GHz
26,581,355,844      instructions              #    1.39  insn per cycle
 6,242,394,037      branches                  #  816.275 M/sec
   128,405,656      branch-misses             #    2.06% of all branches

   7.779848330 seconds time elapsed

With this patch:

   7679.173467      task-clock (msec)         #    0.987 CPUs utilized
           321      context-switches          #    0.042 K/sec
            23      cpu-migrations            #    0.003 K/sec
        86,071      page-faults               #    0.011 M/sec
19,150,248,538      cycles                    #    2.494 GHz
26,667,465,697      instructions              #    1.39  insn per cycle
 6,262,381,898      branches                  #  815.502 M/sec
   128,527,669      branch-misses             #    2.05% of all branches

   7.779477815 seconds time elapsed

(Intel(R) Xeon(R) Platinum 8180M CPU @ 2.50GHz)

The vector insert operation is also be O(n). If the number of non-inherited is expected to be smaller, we can instead search for the first inherited attribute starting at the end of the vector. If we want to avoid the O(n) entirely, we need one list for inherited and another for non-inherited attributes.

aaron.ballman added inline comments.Aug 14 2018, 12:34 PM
lib/AST/DeclBase.cpp
854–859

Thank you for gathering this data -- those perf numbers look reasonable to me. We can refactor for performance later if it ever turns up on the hot path.

This revision is now accepted and ready to land.Sep 20 2018, 1:48 PM