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

Arthur Eubanks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 23 13:24:06 PDT 2022


aeubanks 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
----------------
tejohnson wrote:
> 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
https://reviews.llvm.org/D126228 for changing LTT.

That still seems like a workaround though. I still think that this patch properly addresses the issue discussed in these comments below

```
    // At this point we could remove all type test assume sequences, as they
    // were originally inserted for WPD. However, we can keep these in the
    // code stream for later analysis (e.g. to help drive more efficient ICP
    // sequences). They will eventually be removed by a second LowerTypeTests
    // invocation that cleans them up. In order to do this correctly, the first
    // LowerTypeTests invocation needs to know that they have "Unknown" type
    // test resolution, so that they aren't treated as Unsat and lowered to
    // False, which will break any uses on assumes. Below we remove any type
    // test assumes that will not be treated as Unknown by LTT.

    // The type test assumes will be treated by LTT as Unsat if the type id is
    // not used on a global (in which case it has no entry in the TypeIdMap).
```

If I'm reading it correctly, it's essentially saying that we *have to* remove the type tests for LTT correctness, which is what this patch does. LTT can try and work around type tests that it thinks are for WPD use by ignoring them, but the principled solution is to have WPD remove the type tests. (then maybe we could remove the logic in LTT to try to detect WPD uses of type tests?) And the only way to remove the type tests is to RAUW true to not break the assumes.

We could fix `findDevirtualizableCallsForTypeTest` to better find corresponding assumes for type tests, but that's not really the problem here, the problem is that we need to make sure we delete all relevant type tests. Improving `findDevirtualizableCallsForTypeTest` would also have to account for not deleting the same assume multiple times for when multiple type tests map to an assume.

I'm not familiar with WPD, but perhaps a mechanism that doesn't rely on type.test + assumes would make sense, like an intrinsic that's a combined type.test + assume that's only meant to be used for WPD?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089



More information about the llvm-commits mailing list