[PATCH] D60089: [ARM] Don't replicate instructions in Ifcvt at minsize

Sat Apr 6 16:25:42 PDT 2019

dmgreen added inline comments.

================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:1939
+    if (TBB.pred_size() != 1 || FBB.pred_size() != 1)
+      return false;
+  }
----------------
efriedma wrote:
> Maybe explain why this increases codesize in a comment.  I guess the issue here is that if a block has multiple predecessors, you essentially have to clone it, then if-convert the clone?  (Theoretically, if we if-convert all the predecessors, it could actually save codesize, but I guess that's unlikely to come up in practice.)
I kind of forgot about A32 code. I've made this T32 only and updated the comment.

>From what I've seen, we usually trade a branch for a IT block, so the cloning comes out as a negative.

================
Comment at: llvm/test/CodeGen/Thumb2/ifcvt-minsize.ll:17
+; CHECK-NEXT:    pop {r4, pc}
+; CHECK-NEXT:  .LBB0_2: @ %if.end
 ; CHECK-NEXT:    bl fn
----------------
efriedma wrote:
> The testcases don't really seem to demonstrate the point of this... yes, you're saving two bytes, but only because we can use cbz here.
Oh yeah. Block placement is duplicating certain instructions it shouldn't too. It gets a taildup limit of 1, which in these situations where you don't get a fallthrough makes things worse.

I tried just turning that off, (putting the limit down to 0 at minsize) but that makes things larger in places.

CBZ's are a part of what makes this better, but I've added a new test that compares against 1

================
Comment at: llvm/test/CodeGen/Thumb2/ifcvt-minsize.ll:48
+; CHECK-NEXT:  .LBB1_2: @ %f
 ; CHECK-NEXT:    bx lr
 entry:
----------------
efriedma wrote:
> I guess this is sort of orthogonal to your patch, but why aren't we generating "pop {r7, pc}" here?
Yet another things that isn't tuned for minsize...

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

https://reviews.llvm.org/D60089