[PATCH] D98884: [IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction

Sat Apr 10 09:27:05 PDT 2021

madhur13490 added a comment.

In D98884#2680577 <https://reviews.llvm.org/D98884#2680577>, @arsenm wrote:

> In D98884#2678593 <https://reviews.llvm.org/D98884#2678593>, @madhur13490 wrote:
>
>> This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.
>>
>> Debugging:
>>
>> 1. Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
>> 2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
>> 3. Running valgrind also did not reveal any new things.
>> 4. Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.
>>
>> However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.
>>
>> Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.
>
> This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?

There is no failing IR but the failing binary. Tablegen constitutes around ~200 C/C++ files and all I know is that the binary is misompiling. It is pretty difficult to say at this point if one single file is miscompiling or a bunch of them. All I know at this point is

1. A lot of new functions are now added to IPSCCP's radar as hasAddressTaken() for them is 'false' now as it meets the condition my patch adds i.e all users of bitcast operator are CB. See llvm::canTrackArgumentsInterprocedurally(Function *F) which is called from IPSCCP.
2. IPSCCP may not be the culprit but it is definitely a starting point of my investigation. There are obvious places in it which needs fix e.g. properly removing argmemonly and inaccessiblemem_or_argmemonly attributes here <https://github.com/llvm/llvm-project/blob/17cf2c94230bc107e7294ef84fad3b47f4cd1b73/llvm/lib/Transforms/Scalar/SCCP.cpp#L2012>.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98884