[PATCH] D81835: [SimplifyCFG] Fix inconsistency in block size assessment for threading

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.

LG as well. The phis are a threading opportunity, not a cost.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2198
+    // We insert PRE Phis purely for threading, so Phis should not be
+    // accounted in block's size.
+    if (!isa<PHINode>(I))
----------------
This comment is a bit confusing to me. I would reason the other way around here and say "We will delete Phis while threading, so Phis should not be accounted in block's size".


================
Comment at: llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll:17
 
 define void @caller1(i1 %c, i64* align 1 %ptr) {
 ; ASSUMPTIONS-OFF-LABEL: @caller1(
----------------
mkazantsev wrote:
> nikic wrote:
> > This test should be adjusted with extra store -1 to retain the previous behavior, otherwise it doesn't show what it's supposed to be showing.
> Ok.
The changes in this test will probably have gone away due to a change to alignment assumptions that has landed in the meantime.


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

https://reviews.llvm.org/D81835