[PATCH] D96083: [LTT] Don't attempt to lower type tests used only by assumes

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:2067
+      // with the DropTypeTests flag set.
+      bool OnlyAssumeUses = !CI->use_empty();
+      for (auto CIU = CI->use_begin(), CIUE = CI->use_end(); CIU != CIUE;) {
----------------
pcc wrote:
> If the intrinsic is unused then we don't want to rewrite it either so you can initialize this to true.
I tried that but it broke a half dozen or so LTT tests (e.g. llvm/test/Transforms/LowerTypeTests/layout.ll) where there were type tests and no uses of the results. The other possibility was to change those tests, but I thought with this initial value it would simplify creating tests. WDYT?


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:2068
+      bool OnlyAssumeUses = !CI->use_empty();
+      for (auto CIU = CI->use_begin(), CIUE = CI->use_end(); CIU != CIUE;) {
+        if (auto *AssumeCI = dyn_cast<CallInst>((*CIU++).getUser())) {
----------------
pcc wrote:
> You can use a standard range for loop. We aren't rewriting users so you don't need this pattern here.
Will fix


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:2069
+      for (auto CIU = CI->use_begin(), CIUE = CI->use_end(); CIU != CIUE;) {
+        if (auto *AssumeCI = dyn_cast<CallInst>((*CIU++).getUser())) {
+          Function *F = AssumeCI->getCalledFunction();
----------------
pcc wrote:
> You can dyn_cast to IntrinsicInst to reduce the amount of boilerplate here.
Will fix (here and in another place in this file that has the same pattern)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96083