[PATCH] D99362: Verify the LLVMContext for Attributes.

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D99362#2696039 <https://reviews.llvm.org/D99362#2696039>, @nikic wrote:

> This change has a noticable compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=8f683366afcfd7c03faea615d1529a0014d7dbde&to=244d9d6e41db71e76eeb55e56d84f658b3f56681&stat=instructions
>
> Only LTO is affected, I believe because release builds only run the verifier when importing LTO bitcode. Link time for tramp3d-v4 increases by 1%, which seems rather excessive for a verifier addition.

Agreed, that seems too high!

> Something to keep in mind is that FoldingSet lookups are quite expensive, and you're doing a lot of them here, because you will be re-checking the same attributes/sets/lists across many functions.

I left a comment inline; I think there's a way to reduce the number of lookups.



================
Comment at: llvm/lib/IR/Verifier.cpp:1885-1888
+    for (const auto &A : AttrSet) {
+      Assert(A.hasParentContext(Context),
+             "Attribute does not match Module context!", &A);
+    }
----------------
Can the verifier remember in a DenseSet if it has already checked this AttrSet / Attr / etc.?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99362