Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dcompute] add support for OpenCL image I/O #3835

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

thewilsonator
Copy link
Contributor

No description provided.


auto sd = ptr->type->isTypeStruct();
auto mod = sd ? sd->sym->getModule() : nullptr;
if (mod && mod->md && mod->md->id == Id::opencl) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is where you hardcode that the module name must be opencl. Should this not be ldc.opencl ? That way, we can ship LDC with that hardcoded module.
Or am I misunderstanding things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be ldc.opencl?

Alas it must be, the SPIR-V backend depends on exclusively the type e.g. opencl.image1d_ro_t and I don't know how to force the name of a type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would work if you put module opencl; at the top of the file, while still having it in ldc/opencl.d.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, druntime/src/ldc would have to be on the import path in addition to druntime/src.

I'll do that in a separate PR, because only a few of the types are needed to test the code generation and I've not finished the file yet, I still need to figure out which combinations of {1d,2d,3d}, {ro,wo,rw}, {array, msaa, depth, buffer,(none)} are valid and the msaa ones are only valid with OpenGL sharing or something.

(Also I still don't know how submodule update PRs work.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, druntime/src/ldc would have to be on the import path in addition to druntime/src.

That would be a big change (because then suddenly import attributes; would 'work').
I would expect the type names to be generated from the name specified by module <name>, rather than the how it is imported using import <name>.
Zooming out a little, I think requiring that the type name in D is exactly how LLVM wants it internally is very brittle. For example, if LLVM expects a different type name, then all user code would have to change aswell (perhaps solvable with a bunch of stable alias types in LDC's opencl.d?). I'd say first priority is getting things to work, but soon after there needs to be some thought behind the future/longer term stability of this functionality working.

I'll do that in a separate PR

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a big change (because then suddenly import attributes; would 'work')

Indeed. We could have it along side object.d, I currently have it under dcompute/source

requiring that the type name in D is exactly how LLVM wants it internally is very brittle

It may be brittle, but it is exceedingly unlikely to change (SPIR (n.b. not SPIR-V) is a standard after all) unless we change backends (which I would love to do, but there is not official SPIR-V that is part of the LLVM project) in which case they would probably all be i8 addrspace(1)* to intrinsic functions which would be completely handled by the dcompute library.

perhaps solvable with a bunch of stable alias types in LDC's opencl.d?

Of course, there's no point calling it an GlobalPointer!image1d_ro_t when you can alias it to ReadOnlyImage!1.
The only mildly annoying concern is the conditionally declared types that depend on if OpenGL image sharing is enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be ldc.opencl?

Yeah, that would be much nicer indeed IMO, keeping extra LDC stuff in its package.

I don't know how to force the name of a type.

The name is currently initialized and then untouched AFAICT in

LLStructType::create(gIR->context(), ad->toPrettyChars())),
. You could override it in

ldc/ir/irtypestruct.cpp

Lines 49 to 55 in f9ba172

IrTypeStruct *IrTypeStruct::get(StructDeclaration *sd) {
IF_LOG Logger::println("Building struct type %s @ %s", sd->toPrettyChars(),
sd->loc.toChars());
LOG_SCOPE;
auto t = new IrTypeStruct(sd);
getIrType(sd->type) = t;
(before returning for opaque structs, as all of these magic structs are opaque at the moment).

Also I still don't know how submodule update PRs work.

You push a branch onto the LDC repo and then reference it here in an LDC PR. After merging the PR here, the druntime branch needs to be merged into druntime's ldc branch (and must be a fast-forward merge, i.e., the druntime branch must branch off the current ldc head).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could override it before returning for opaque structs

Thanks, will do some investigation tomorrow about this.

as all of these magic structs are opaque at the moment

correct, and I don't see that changing any time soon.

You push a branch onto the LDC repo and then reference it here in an LDC PR. After merging the PR here, the druntime branch needs to be merged into druntime's ldc branch (and must be a fast-forward merge, i.e., the druntime branch must branch off the current ldc head).

THANKS!!! will bookmark this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@kinke kinke Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use isFromLDC_OpenCL(sd) for the if-condition now, that would simplify things (and restrict it to ldc.opencl only).

@thewilsonator
Copy link
Contributor Author

Any final comments? It would be good to get this in before v1.28 is tagged.

@@ -38,6 +39,7 @@

namespace {
class TargetOCL : public DComputeTarget {
bool usedImage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you read from it but don't set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 1f24be6 stackoverflow tells me that s.rfind("needle", 0) == 0 is how you're supposed to do startsWith on a string prior to C++20.

@thewilsonator
Copy link
Contributor Author

Hmm, I seem to have a merge conflict that I can't figure out how to fix.

@thewilsonator thewilsonator force-pushed the cl-images-2 branch 3 times, most recently from f6f2cdc to 1002c90 Compare September 27, 2021 04:00
@thewilsonator
Copy link
Contributor Author

Hmm, the druntime commit seems to be

-e1ba20fec08568c9f7d81ceedbac84d78cfdcd34
+e3738e482e9854cf7eb05c8f79abe5405fd1a8a9

instead of

-06fdf5464471d9877bce2aeadc80d527b7e8db1d
+e3738e482e9854cf7eb05c8f79abe5405fd1a8a9

hence the merge conflict. I've no idea why its doing that though or how to fix it.

@thewilsonator
Copy link
Contributor Author

thewilsonator commented Sep 27, 2021

Yay, it worked! Nope the submodule changes are wrong.

@kinke
Copy link
Member

kinke commented Sep 27, 2021

That's probably caused by master pointing to a newer druntime hash (I've merged upstream stable recently) than when this branch was branched off - rebase this branch in that case (and yes, it's going to complain about a druntime conflict, just commit your change, your druntime branch is fine).

@thewilsonator thewilsonator force-pushed the cl-images-2 branch 2 times, most recently from b162c2b to a5b78ed Compare September 27, 2021 05:07
@thewilsonator
Copy link
Contributor Author

Yay! I've still got some fixes to do on the druntime bit.

@kinke
Copy link
Member

kinke commented Sep 27, 2021

You could try making ldc.opencl a .di - it has opaque structs and aliases only, shouldn't need to be compiled (no init symbols etc., neither for GlobalPointer!… instantiations as they are most likely zero-initialized).

@thewilsonator
Copy link
Contributor Author

That fixes it locally.

@thewilsonator
Copy link
Contributor Author

Ubuntu 16.04 x64 ltsmaster fails with:

Unable to checkout 'f2a6ae9802aceef9e958ac0d62b0128b85c1aeae' in submodule path 'runtime/druntime'

is that a problem with ltsmaster CI?

@kinke
Copy link
Member

kinke commented Sep 28, 2021

I've just retried it (thought you might have pushed the druntime commit a bit too late after pushing here), but it failed identically. Might have to do with an old git of that vanilla Ubuntu 16 image and some strange --depth 50 shallow-cloning behavior. The job is new, so no history of this problem so far.

@kinke
Copy link
Member

kinke commented Sep 28, 2021

You could try removing the --depth bit in

ldc/.cirrus.yml

Line 186 in f9ba172

git submodule update --init --depth $CIRRUS_CLONE_DEPTH

@thewilsonator
Copy link
Contributor Author

Hmm, didn't work.

@kinke
Copy link
Member

kinke commented Sep 28, 2021

You've patched the wrong line.

@thewilsonator
Copy link
Contributor Author

Thanks, that seems to have fixed it.

For unknown reasons this can result in
"Unable to checkout 'hash' in submodule path 'runtime/druntime'"
@thewilsonator
Copy link
Contributor Author

I think this is good to go.

@kinke
Copy link
Member

kinke commented Sep 30, 2021

[As for druntime, I suppose you could get rid of @compute(CompileFor.deviceOnly) as it's a .di anyway.]

@thewilsonator
Copy link
Contributor Author

[As for druntime, I suppose you could get rid of @compute(CompileFor.deviceOnly) as it's a .di anyway.]

I think it serves a purpose as documentation, its probably not necessary but does no harm. Anyway this is good to go from my perspective.

@thewilsonator
Copy link
Contributor Author

Good to go?

@kinke kinke merged commit b09d4be into ldc-developers:master Oct 11, 2021
@thewilsonator
Copy link
Contributor Author

Thanks!

@thewilsonator thewilsonator deleted the cl-images-2 branch August 1, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants