[PATCH] D126689: [IR] Enable opaque pointers by default

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added a comment.

In D126689#4654126 <https://reviews.llvm.org/D126689#4654126>, @nikic wrote:

> In D126689#4654124 <https://reviews.llvm.org/D126689#4654124>, @uabelho wrote:
>
>> @nikic: Thanks, nothing to do there then even if I'm surprised.
>>
>> I'm not sure but I *think* that llvm-reduce may have some problems with this. For my out of tree target I recently saw a case where llvm-reduced crashed with
>>
>>   llvm-reduce: ../tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:64: void replaceFunctionCalls(llvm::Function *, llvm::Function *): Assertion `CI->getCalledFunction() == OldF' failed.
>>
>> and when I looked at the reduced result so far, I saw a call where parameters didn't match the declaration. So I guess it may now reduce in ways that it unexpected for it and then crash.
>
> Can you please file an issue for the llvm-reduce bug? I just took a quick look at the code, and it indeed has a mismatch in checks between canReplaceFunction() and replaceFunctionCalls() -- the conditions in both need to be the same, but aren't.

Yeah I can do that. Unfortunately I don't have any reproducer I can share though but if you think you know at least one problem in the vicinity maybe it's good enough.

In D126689#4654126 <https://reviews.llvm.org/D126689#4654126>, @nikic wrote:

> In D126689#4654124 <https://reviews.llvm.org/D126689#4654124>, @uabelho wrote:
>
>> @nikic: Thanks, nothing to do there then even if I'm surprised.
>>
>> I'm not sure but I *think* that llvm-reduce may have some problems with this. For my out of tree target I recently saw a case where llvm-reduced crashed with
>>
>>   llvm-reduce: ../tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:64: void replaceFunctionCalls(llvm::Function *, llvm::Function *): Assertion `CI->getCalledFunction() == OldF' failed.
>>
>> and when I looked at the reduced result so far, I saw a call where parameters didn't match the declaration. So I guess it may now reduce in ways that it unexpected for it and then crash.
>
> Can you please file an issue for the llvm-reduce bug? I just took a quick look at the code, and it indeed has a mismatch in checks between canReplaceFunction() and replaceFunctionCalls() -- the conditions in both need to be the same, but aren't.

Sure, I wrote
 https://github.com/llvm/llvm-project/issues/69312
which is pretty useless since I can't share any reproducer but anyway there it is.
Good if you saw something in the vicinity that indeed looks related.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689