support ptx code type for program #317
Conversation
|
/ok to test |
leofang
added
enhancement
…amOptions change to go in
|
/ok to test |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
|
/ok to test |
ksimpson-work
force-pushed
the
support-ptx-program
branch
from
587b5d0 to
4b704fa
Compare
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
| link_time_optimization=options.link_time_optimization, | ||
| split_compile=options.split_compile, | ||
| ) | ||
|
|
||
| def close(self): | ||
| """Destroy this program.""" | ||
| self._mnff.close() |
There was a problem hiding this comment.
| self._mnff.close() | |
| self._mnff.close() | |
| if self._linker: | |
| self._linker.close() |
There was a problem hiding this comment.
Is there an advantage to explicitly closing the Linker instance from the Program finalizer rather than letting Program decrement the Linker instance refcount and relying on the established logic in Linker? My instincts tell me to rely on the python runtime garbage collection scheduling for PyObjects.
I've changed the mnff handle attribute name to nvrtc_handle to be more explicit, since before it felt implied that you were referencing a handle that was inherent to the Program instance, rather than a backend dependent handle. Let me know what you think. Functionally there is no difference
There was a problem hiding this comment.
The semantics of close() is: Destruct every resource that this object owns immediately, and consider this object no longer usable after close() returns. It does not rely on the gc behavior, which is the last resort (gc can defer the destruction at an arbitrarily later time). We need this guarantee for certain use cases.
There was a problem hiding this comment.
It is a good catch about Program.handle. I think it should return either nvrtc_handle or linker.handle (which is either nvjitlink handle or cuLink handle). For clarity on the semantics of the handle, we should make Linker.backend queryable (it is not today), so that Program.backend returns Linker.backend if it's using the linker under the hood. Then we teach users about "check which backend is in use to determine how to interpret the handle". WDYT?
We should fix this asap but perhaps in a separate PR. Would you mind creating a new issue to track this?
There was a problem hiding this comment.
I follow your point for the close() method. I will implement it as you suggest.
For the handles, I'm a bit torn. It makes sense that we would return a handle / backend of equivalent level of abstraction to that of nvrtc ( return a handle, or the binding level backend ), however it also feels like we are breaking the Linker's abstraction by doing so. An nvrtc backed Program's handle is an ~nvrtcProgramHandle, while a linker backed Program doesn't really have a handle. It has a linker backend which has a culinkHandle or nvjitLinkHandle which is a handle to the linker rather than a program. Tradeoff between handle semantic consistency and abstraction consistency.
The same tradeoff feels relevant to the return value of Program.backend (either Linker, or Linker.backend).
The only thing I feel sure of, is that we should put this in a new issue and discuss it on a call. In the meantime I will revert the handle name change I made so this review as isolated as possible from that discussion. #433 is the issue
|
Let's also remove this TODO line: |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
support PTX as a code type for Program. This is achieved by using the linker as a backend for Program when the input type is PTX.
This includes a translation from program options to linker options in the program class. Which inspired the removal of the
Maps tosection of option documentation for linker and program which now both sometimes map to other things that what was documented. (linker to either nvjitlink or culink and program to either nvrtc, nvjitlink or culink)This also inspired the handling of deprecated options in culink to raise warnings instead of errors. This allows a user to write cuda which targets a deprecated option if they are developing on an older system and deploying on a newer one for example.
close #284