[PATCH] D64101: [LoopUnroll] fix cloning callbr

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D64101#1569682 <https://reviews.llvm.org/D64101#1569682>, @hfinkel wrote:

> In D64101#1569642 <https://reviews.llvm.org/D64101#1569642>, @efriedma wrote:
>
> > Or actually, it might make more sense to change the way we generate/lower callbr, to make the label parameters implicit; instead of modeling them with blockaddress, we never write them in IR at all, and automatically generate them later based on the successor list.
>
>
> That's an appealing idea.


I kind of agree; looking at the arguments to callbr, the label list (the final part in `[]`) feels like duplicate information of the earlier `blockaddress` operands.  The `blockaddress` operands in the common case SHOULD match or be the address of blocks in the label list, except when you're explicitly passing the address of labels around (see below link).  In that first case, why specify explicitly the blockaddress at all?  Surely other transforms may update the label list but not the blockaddresses, just like this patch w/ LoopUnroll.  I feel like the blockaddress could be implicit based on the label list, and generated as late as possible and only when needed.

That said, I'd really like to land this patch as it addresses an observable and bad bug in the Linux kernel, and I'd like to do so to make the clang-9 release train, rather than try to rearchitect `callbr` here.

> We do need to be careful, AFAIK, because the goto targets can be passed into the asm as parameters (where the block addresses come from the labels-as-values extension). However, in theory we already shouldn't unroll loops with blocks with their address taken, so I imagine that case is irrelevant here (*).
> 
> (*) LoopUnroll.cpp actually only seems to check Header->hasAddressTaken(), not all of the blocks, so maybe that's wrong?

Great point.  Looking at GCC:
https://godbolt.org/z/lKa_HD

Notice: it does unroll/duplicate the label within the loop when the label is passed in via the final label list in the asm goto.  It **does not** duplicate the address of label passed in, even when the induction variable is captured.

So my patch as it stands in the current revision is incorrect or will differ from GCC; I need to differentiate between is this `BlockAddress` operand explicitly passed in as an input, or is it from the label list.  I don't think it's hard for me to change what I have to at least match GCC.  To @eli.friedman 's point; then yes `blockaddress` is indeed handled differently in this case.  Let me fix up the patch and add a test based on the godbolt link.

And thanks to everyone so far for the feedback and discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64101