Page MenuHomePhabricator

[OpaquePtr] Mangle intrinsics with opaque pointers arguments
ClosedPublic

Authored by zequanwu on Jun 14 2021, 3:05 PM.

Details

Summary

Mangling intrinsics with opaque pointer arguments using "op"+{address space}.

Diff Detail

Event Timeline

zequanwu created this revision.Jun 14 2021, 3:05 PM
zequanwu requested review of this revision.Jun 14 2021, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2021, 3:05 PM

there needs to be tests, and the description should probably state what's going on

zequanwu updated this revision to Diff 352012.Jun 14 2021, 3:59 PM

Add a test case.

zequanwu edited the summary of this revision. (Show Details)Jun 14 2021, 4:00 PM

maybe we should make a quick post on llvm-dev about this to see what other people think, since this will be very user-visible

llvm/lib/IR/Verifier.cpp
1843 ↗(On Diff #352012)

we still want some of the checks below, for example, we want to check !Attrs.getByValType()->isSized()
perhaps we should separate this out into a separate change

(generally looks like the right direction to me - sounds like @aeubanks has some good feedback/issues to address)

zequanwu updated this revision to Diff 352263.Jun 15 2021, 3:00 PM
zequanwu edited the summary of this revision. (Show Details)

Rebase.

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

llvm/test/Verifier/opaque-ptr.ll
34

This is actually not the correct name, it should end with op0. We should add a CHECK line for the actual name.

zequanwu updated this revision to Diff 352280.Jun 15 2021, 4:05 PM
zequanwu marked an inline comment as done.

Address comment.

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

IIUC, we don't really care about the exact name, just that it doesn't conflict with another declaration of the same intrinsic but with different parameter types. This is unambiguous even when non-opaque pointers are in the picture, since non-opaque pointers will always have something after the p0.

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

It seems what's after "{intrinsic}." is not important for IR parsing: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/IntrinsicInst.cpp#L127.

aeubanks accepted this revision.Jun 15 2021, 4:43 PM

lgtm, now that I understand intrinsics a bit more, this seems trivial enough to not need input from llvm-dev

This revision is now accepted and ready to land.Jun 15 2021, 4:43 PM

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

It seems what's after "{intrinsic}." is not important for IR parsing: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/IntrinsicInst.cpp#L127.

Though currently the mangling scheme does allow arbitrary overloading, I think, yeah? If we don't allow arbitrary overloading of intrinsics with the same name, but only use the mangling to fix different parameter types while always having the same number of parameters, then it's probably fine. (then we'd only hit ambiguities in really niche cases maybe?)

But with this change we couldn't overload some intrinsic with "(ptr, int)" and "(ptr)" because they'd produce the same mangling in the case where the former was an opaque pointer and the latter was a typed int pointer?

But we probably don't actually do that?

I couldn't parse your comment, could you rephrase?

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

So, it is not ambiguous. Does the patch look good to you?

dblaikie accepted this revision.Jun 16 2021, 10:28 PM

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

So, it is not ambiguous. Does the patch look good to you?

Yep yep, thanks!