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

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

uabelho added a comment.

In D126689#4654126 , @nikic wrote:

> In 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 , @nikic wrote:

> In 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

uabelho added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@uabelho The call is well-defined from an LLVM IR perspective, so the verifier 
cannot reject it, just as it can't reject calling convention or ABI attribute 
mismatch. In this specific case, the call is likely undefined behavior, but 
that is not generally the case just because there is a signature mismatch. When 
exactly this is UB is still something of an open question (and I believe 
currently effectively boils down to "does the signature match after lowering or 
not").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

uabelho added a comment.
Herald added subscribers: gysit, Dinistro, bviyer, jplehr, Moerafaat, 
kmitropoulou, zero9178, StephenFan.
Herald added a reviewer: dcaballe.

Hi @nikic,

I know I'm very late to the party with this, but I just noticed that since 
opaque pointers got turned on by default, the following program does not yield 
an error anymore when read by opt or llc:

  declare void @bar(i16)
  
  define void @foo() {
  entry:
call void @bar(float 1.00e+0)
ret void
  }

With typed pointers opt and llc would fail directly with

  opt: test.ll:5:13: error: '@bar' defined with type 'void (i16)*' but expected 
'void (float)*'
call void @bar(float 1.00e+00)
  ^

but now, since all pointers are just pointers, noone seems to bother about the 
mismatching parameter type.

The verifier doesn't complain either. Maybe it should be improved to explicitly 
check parameter types now that we won't get that for free via the function 
pointer type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

uabelho added a comment.

In D126689#3553292 , @nikic wrote:

> @uabelho Here's a slightly cleaned up test case that does not use opaque 
> pointers:
>
>   target triple = "x86_64-unknown-linux-gnu"
>   
>   define void @test(i1 %cond, <1 x i16>* %p) {
> br label %loop
>   
>   loop:
> %v = load <1 x i16>, <1 x i16>* %p, align 2
> %ins = insertelement <4 x double> zeroinitializer, double 0.00e+00, 
> i32 0
> %cmp = fcmp uge <4 x double> %ins, zeroinitializer
> %ashr = ashr <1 x i16> %v, %v
> %shuf = shufflevector <4 x i1> %cmp, <4 x i1> zeroinitializer, <4 x i32> 
> zeroinitializer
> br i1 %cond, label %loop, label %exit
>   
>   exit:
> %use1 = add <4 x i1> %shuf, zeroinitializer
> %use2 = add <1 x i16> %ashr, zeroinitializer
> ret void
>   }
>
> llvm-stress can probably generate certain code patterns when opaque pointers 
> are enabled, which it does not produce when typed pointers are used. In this 
> case at least, the used pointer type doesn't matter in the end.

Yes llvm-stress must generate something unusual with opaque pointers because 
normally I very rarely see that it finds something, and after the switch done 
here I get several different crashes. The one I posted above was just one 
example.
Next crash here: https://github.com/llvm/llvm-project/issues/55846


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

I was able to update LLDB 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/44252/. We can leave 
this in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

It looks like this patch has broken 168 tests in LLDB: 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/44239/changes#41d5033eb162cb92b684855166cabfa3983b74c6
I'm going to dig a little deeper, but I might ask you to revert this until we 
can figure out a solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

I believe https://reviews.llvm.org/D126886 should fix the DAGCombiner issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@uabelho Here's a slightly cleaned up test case that does not use opaque 
pointers:

  target triple = "x86_64-unknown-linux-gnu"
  
  define void @test(i1 %cond, <1 x i16>* %p) {
br label %loop
  
  loop:
%v = load <1 x i16>, <1 x i16>* %p, align 2
%ins = insertelement <4 x double> zeroinitializer, double 0.00e+00, i32 0
%cmp = fcmp uge <4 x double> %ins, zeroinitializer
%ashr = ashr <1 x i16> %v, %v
%shuf = shufflevector <4 x i1> %cmp, <4 x i1> zeroinitializer, <4 x i32> 
zeroinitializer
br i1 %cond, label %loop, label %exit
  
  exit:
%use1 = add <4 x i1> %shuf, zeroinitializer
%use2 = add <1 x i16> %ashr, zeroinitializer
ret void
  }

llvm-stress can probably generate certain code patterns when opaque pointers 
are enabled, which it does not produce when typed pointers are used. In this 
case at least, the used pointer type doesn't matter in the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

uabelho added a comment.

Is anyone here ever using llvm-stress?
I noticed that with this patch, llvm-stress seems to start producing code that 
llc crashes on, and it crashes even if I run on older llc versions.
One example:

  llc -o /dev/null stress.ll

crashes with

  llc: ../lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5974: llvm::SDValue 
llvm::SelectionDAG::getNode(unsigned int, const llvm::SDLoc &, llvm::EVT, 
llvm::SDValue, llvm::SDValue, const llvm::SDNodeFlags): Assertion 
`VT.isInteger() && N2.getValueType().isInteger() && "Shifts only work on 
integers"' failed.

Or is the X86 backend just not up to date with opaque pointers yet?
F23291141: stress.ll 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits