[PATCH] D126089: [WPD] Make sure type test is eliminated

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1929
+      if (!CI->use_empty())
+        CI->replaceAllUsesWith(ConstantInt::getTrue(M.getContext()));
       // We can't use RecursivelyDeleteTriviallyDeadInstructions here because we
----------------
aeubanks wrote:
> tejohnson wrote:
> > The equivalent code in LowerTypeTests::lower() asserts that the users are all phi nodes in this case. Can you add the same check here?
> that asserts on [1]. I'm not sure why the assert in LTT never triggers since it does seem like you could potentially do arbitrary things with the result of type tests. if we really wanted to make sure that type tests were only used by assumes, perhaps indirectly through phis, I think that would be more of a verifier check
> 
> [1] https://github.com/llvm/llvm-project/blob/8801a5d185fafcb8c03f71ceb717b84c2f08b220/llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll#L32
> that asserts on [1].

Ah, ok, the assert is too aggressive. But I believe the code change here is as well - for the type test in [1] with the current code LTT will presumably lower it to a false but with this patch it is a true (which I don't think the test is checking, since it is focused on something else, but I'm not sure this code should be replacing all type tests it encounters here with true).

> I'm not sure why the assert in LTT never triggers 

Because the only type tests that survive the first LTT call are specifically type tests that directly feed assumes [2]. So the only way for it to not feed an assume by the second LTT invocation is if inst combine or the like merged 2 assumes in which case the non-assume use must be a phi.

[2] https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L2098-L2112

So perhaps this code should look through uses on phis to ensure that the only uses are on assumes. Except, this raises the question of why the assume in the original code was not removed by the loop just above here at lines 1924-1925. Aha - it is because the code that populates this list has the same limitation - looking for type tests that directly feed assumes [3]. Perhaps that is the code that should be modified to look through phis? Because we may be missing out on WPD opportunities by not even considering these.

[3] https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/llvm/lib/Analysis/TypeMetadataUtils.cpp#L82-L85


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089