[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson marked 2 inline comments as done.
tejohnson added a comment.

PTAL



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1999
+    WholeProgramDevirtResolution *Res = nullptr;
+    if (ExportSummary && isa<MDString>(S.first.TypeID))
+      // Create the type id summary resolution regardlness of whether we can
----------------
tejohnson wrote:
> The fix needed for the Chromium issue was to guard the TypeIdSummary creation here by whether the TypeID exists in the TypeIdMap (which makes it match the comment in fact), as we don't want to create a summary if the type id is not used on a global (in which case it should in fact be Unsat). The equivalent code in the index-only WPD is essentially already guarded by that condition, because of the way the CallSlots are created (and in fact there is an assert in that code that we have a use on a vtable, i.e. that a TypeIdCompatibleVtableSummary is found for the TypeID).
Improved the fix I had made here for Chromium as it wasn't robust enough to handle all cases. Specifically, if we came through this code multiple times with the same type id, the TypeIdMap entry would no longer be missing since we unconditionally created it in the call to tryFindVirtualCallTargets below (via the operator[]). Changed it to check for a non-empty set (after more eagerly calling operator[]). I beefed up the test I had for this issue (llvm/test/ThinLTO/X86/cfi-unsat.ll) to cover this additional case.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1784
+          ImportSummary->getTypeIdSummary(cast<MDString>(TypeId)->getString());
+      if (!TidSummary)
+        RemoveTypeTestAssumes();
----------------
Here is the fix for the issue with the multi-stage clang bootstrap test failures described in D75201. I also restructured this code to try to make it clearer (rather than an early continue when we don't need to remove type test assumes, make it an explicit removal when needed).

Added new test llvm/test/ThinLTO/X86/type_test_noindircall.ll for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242