[PATCH] D84985: [ThinLTO] Compile time improvement to propagateAttributes

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D84985#2187270 <https://reviews.llvm.org/D84985#2187270>, @evgeny777 wrote:

>> I made a smaller efficiency improvement (no measurable impact) to skip all summaries for a VI if the first copy is dead. I added an assert to ensure that all copies are dead if any is....
>
> Will this work correctly in case of GUID collision? (see https://reviews.llvm.org/D67322)

The code in computeDeadSymbols will conservatively mark all copies live if any is. See not only the worklist iteration I modified here, but also the code in visit at lines 878-879, and the preserved GUID handling at lines 814-815. So if there is a collision, the colliding values may conservatively be marked live, so the loop in propagateAttributes will handle them.  I.e they are either all dead or all live (especially after my fix here which will make the behavior more conservative in the chance case of any alias values that collide with a value marked live during module summary building). Other than the corner case I'm fixing here, this should be a no-op in behavior for that loop in propagateAttributes, which was already skipping all dead values, and which does check if it is a GlobalVarSummary already.

> BTW, what's the maximum performance improvement you got in absolute values?

In a large binary build I saw this shave off ~10-11s in a ~71s thin link.



================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:179
     assert(VI.getAccessSpecifier() == 0 || isa<FunctionSummary>(S));
+    if (!VI.isReadOnly() && !VI.isWriteOnly()) {
+      if (!MarkedNonReadWriteOnly.insert(VI).second)
----------------
evgeny777 wrote:
> nit: you can check for `!VI.getAccessSpecifier()` here and probably refactor the assertion above
Will change the check here. Not sure what you want me to do with the assert though? There's not a convenient else below to put an assertion that just checks the isa<> when the access specifier is null, since it is an else if.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84985/new/

https://reviews.llvm.org/D84985