<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">================<br>
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:236<br>
@@ +235,3 @@<br>
+                                      Value *BitOffset) {<br>
+  if (BSI.Bits.size() <= 8) {<br>
+    // If the bit set is sufficiently small, we can avoid a load by bit testing<br>
----------------<br></span><span class="">> Maybe I have a naive view, but shouldn't bitsets be GC'able if no test refers to them? This doesn't need to be an optimization that's specific to your code, LLVM can do this in general when a global doesn't escape and isn't address-taken (and in your case, is read-only). If this is correct, then I don't think you need to split up this pass, though I agree that you may want to do devirtualization earlier to expose more optimization opportunities.<br>
><br>
> Under the current setup, do redundant tests in the same function get eliminated and control flow merged?<br>
><br>
> This may be something that we can leave open for later changes: I think the current code is good in that it does what's required and is pretty efficient at it. I don't think the design will change substantially, but I do think there are further optimization opportunities here. WDYT?<br>
> Maybe I have a naive view, but shouldn't bitsets be GC'able if no test refers to them?<br>
<br>
</span>They could be, but the globals that the bitsets map onto (i.e. the vtables) cannot be GC'd because we lay them out in a specific order in this pass.<br>
<br>
We only build bitset constants for bitsets that are referred to by tests. The loop near the start of `LowerBitSets::buildBitSets` identifies all such bitsets by looking through the uses of the `llvm.bitset.test` intrinsic. If a particular test is dead, LLVM should equally be able to remove the dead test (as it is a readonly intrinsic) or remove a dead load from a bitset as part of DCE (in fact the former would probably be easier because of the simpler control flow).<br></blockquote><div><br></div><div>OK, that's what I was hoping happens (I was afraid the optimization was running too early to be able to do this).</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Another advantage of doing this late is that allowing the earlier passes to eliminate dead tests we potentially reduce the number of equivalence classes we need to create, which could result in smaller disjoint sets of classes and therefore smaller bitsets.<br></blockquote><div><br></div><div>Agreed.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> Under the current setup, do redundant tests in the same function get eliminated and control flow merged?<br>
<br>
</span>Are you referring to cases where a virtual call happens twice through the same pointer?<br></blockquote><div><br></div><div>Yes, or any case where the test intrinsic is redundant (doesn't have to be the same f).</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The problem is that it will be difficult to remove redundant tests because of the semantics of C++. In this case the function f could overwrite the memory region that p refers to with an object of a different derived class without invoking undefined behavior. We might want a flag that a user can use to promise that such things will never happen though.<br></blockquote><div><br></div><div>Good point, I hadn't thought through that. It may be worth adding to the design doc?</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> I don't think the design will change substantially, but I do think there are further optimization opportunities here. WDYT?<br>
<br>
</span>At a high level I do agree that there are optimization opportunities to pursue here. (I could elaborate, but this probably isn't the place.)</blockquote><div><br></div><div>OK, overall this lgtm, I think we're on the same page w.r.t. potential optimizations. </div></div></div></div>