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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 accepted this revision.
evgeny777 added a comment.
This revision is now accepted and ready to land.

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

Ok, got it. My understanding is that collision still may happen between internal module asm symbol (because those have `Live = true` after analysis) and some other internal symbol with the same name  (and module name) which is computed dead. This is rather unlikely to happen in reality, but please leave a comment about this before assertion. That said LGTM.



================
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)
----------------
tejohnson wrote:
> 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.
Ah, ok. Let's leave assert where it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84985