This is an archive of the discontinued LLVM Phabricator instance.

[IR] Enable opaque pointers by default
ClosedPublic

Authored by nikic on May 31 2022, 4:01 AM.

Details

Summary

This enabled opaque pointers by default in LLVM. The effect of this is twofold:

  • If IR that contains neither explicit ptr nor %T* types is passed to tools, we will now use opaque pointer mode, unless -opaque-pointers=0 has been explicitly passed.
  • Users of LLVM as a library will now default to opaque pointers. It is possible to opt-out by calling setOpaquePointers(false) on LLVMContext.

A cmake option to toggle this default will not be provided. Frontends or other tools that want to keep using typed pointers temporarily should disabled opaque pointers via LLVMContext.

Diff Detail

Event Timeline

nikic created this revision.May 31 2022, 4:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 4:01 AM
nikic requested review of this revision.May 31 2022, 4:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 4:01 AM
lkail added a subscriber: lkail.May 31 2022, 4:21 AM
nikic updated this revision to Diff 433334.Jun 1 2022, 2:53 AM

Update more tests.

nikic updated this revision to Diff 433408.Jun 1 2022, 8:18 AM
nikic edited the summary of this revision. (Show Details)
nikic added a reviewer: Restricted Project.

Finish updating LLVM, Clang, Polly and MLIR tests.

Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
aeubanks accepted this revision.Jun 1 2022, 11:18 AM
aeubanks added a subscriber: aeubanks.

\o/

We could split out the test changes with -opaque-pointers but I'm not sure how worth it that is given that we'll probably just fix forward any regressions, and rollbacks would probably require updating a couple tests added in the meantime.

This revision is now accepted and ready to land.Jun 1 2022, 11:18 AM
This revision was landed with ongoing or failed builds.Jun 2 2022, 12:41 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 12:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
anna added a subscriber: anna.Jun 2 2022, 6:26 AM

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?

nikic added a comment.Jun 2 2022, 7:32 AM

@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.000000e+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.

aprantl added a subscriber: aprantl.Jun 2 2022, 9:29 AM

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.

uabelho added a comment.EditedJun 2 2022, 10:51 PM

@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.000000e+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 quickly got different crashes. The one I posted above was one example.
Next crash here: https://github.com/llvm/llvm-project/issues/55846
and another (very similar) here: https://github.com/llvm/llvm-project/issues/55848

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.000000e+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.000000e+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?

nikic added a comment.Oct 17 2023, 2:00 AM

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

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

nikic added a comment.Oct 17 2023, 2:40 AM

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

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

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