[PATCH] D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

In D103867#2828561 <https://reviews.llvm.org/D103867#2828561>, @hoy wrote:

> In D103867#2814911 <https://reviews.llvm.org/D103867#2814911>, @wenlei wrote:
>
>> In D103867#2814684 <https://reviews.llvm.org/D103867#2814684>, @lxfind wrote:
>>
>>>> Poking around it looks like this should be handled for inlining, not sure why the previous case we ran into wasn't covered by that code below from `CallAnalyzer::analyze()`
>>>>
>>>>   // Disallow inlining a blockaddress with uses other than strictly callbr.
>>>>   // A blockaddress only has defined behavior for an indirect branch in the
>>>>   // same function, and we do not currently support inlining indirect
>>>>   // branches.  But, the inliner may not see an indirect branch that ends up
>>>>   // being dead code at a particular call site. If the blockaddress escapes
>>>>   // the function, e.g., via a global variable, inlining may lead to an
>>>>   // invalid cross-function reference.
>>>>   // FIXME: pr/39560: continue relaxing this overt restriction.
>>>>   if (BB->hasAddressTaken())
>>>>     for (User *U : BlockAddress::get(&*BB)->users())
>>>>       if (!isa<CallBrInst>(*U))
>>>>         return InlineResult::failure("blockaddress used outside of callbr");
>>>>
>>>>> LGTM, thanks for the fix.
>>>
>>> I think because that code is only checking for CallBrInst users. But in a computed goto pattern, the addresses are usually just stored in a global variable.
>>
>> Taking address of a block and storing it in a jump table does not introduce a use in the IR, however, if we use indirect branch to access the jump table like computed goto, we do count the indirect branch as use of the address taken labels, which in turns blocks inlining. See https://godbolt.org/z/8jYxzq867 (If we comment out the check for disallowing all indirbranch, it would be caught by the check quoted above)..
>
> The following example can be used to tell the difference between GCC and LLVM regarding inlining. GCC won't inline function `foo` into `bar` while clang will. I was thinking this can be fixed in LLVM by enforcing a noinline attribute to functions of which the code labels are used in a static variable initializer. Fixing in the inliner might be better.
>
>   char *g;
>   void go();
>   void   foo(char *p)
>   {
>      static const void* array[] = {&&L};
>   L:
>      if (p) {
>      p--;
>      go();
>      goto L;
>      }
>      return;
>   }
>   void bar()
>   {
>     foo(g);
>   }

Yeah, this case differentiates gcc and llvm, because there's no indir branch. Also since there's no indir branch through the jump table, the inlining isn't buggy. However, as soon as you add indir branch through the jump table, inlining would be blocked in llvm like the case from the godbolt link above. It looks to me that there's attempt/effort try to cover such cases to prevent bad inlining, perhaps it's not strong enough - we can pass jump table address to a callee of foo, then that callee calls the address though indirect branch, so from foo, we still don't see a indirect branch.. The more robust way of preventing such bad inlining is as you said, gate it on storing code label into a static/global variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103867