[PATCH] D65060: [LICM] Make Loop ICM profile aware

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for reply, @asbirlea. Now I see where the non-determinism comes from. I thought that BFI query APIs all go through a translation from BasicBlock* to BlockNode, thus in query APIs, we use BasicBlock* just as a look up key without actually dereferencing it. This is what makes me think removing basic block is fine (we'll have dead entry, but it won't be accessed). But if the dangling BasicBlock* pointer happens to point to newly allocated BasicBlock* later, the count we get for that new block can be non-deterministic. This is quite tricky.. I'm not sure how crash can happen still as it seems we don't dereference BasicBlock* in query APIs, though non-determinism is enough of a problem.

> I think that the most basic update will be to remove all invalid data from the BFI internal data structures. Then, yes, it becomes a matter of accuracy.

Agreed. This is like actually invalidating BFI on loop or block level. Is this being worked on actively? If not, folks from our side or myself can probably do it. For the long term, I still think we need to properly persist the update to metadata though, for the accuracy of later BPI/BFI passes.

> I think the BFI cleanup update would give you the answer to whether the improvements you were seeing were genuine.

The wins we got was with legacy pass manager. (We started evaluating new pass manager in a different context).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65060