PATCH: Make sure NVPTX doesn't emit symbols that aren't valid for ptxas
justin.holewinski at gmail.com
Mon Mar 10 13:20:17 PDT 2014
On Mon, Mar 10, 2014 at 4:15 PM, Eli Bendersky <eliben at google.com> wrote:
> On Mon, Mar 10, 2014 at 12:26 PM, Justin Holewinski <
> justin.holewinski at gmail.com> wrote:
>> I had to ponder the patch a bit before I realized it was a reverse patch
>> and you were adding the getSymbolName function. I was trying to figure out
>> why we had an issue in the first place if this function already existed,
>> any why you were reverting it to go back to the Mangler interface. :)
> Oops, sorry about the reverse patch. This is now committed in r203483.
>> The patch LGTM. I'm still concerned about collisions, but I agree that
>> this is better than what we currently have.
>> An alternative could be to implement a pre-codegen IR pass that renames
>> globals to match what is representable in PTX. This would not incur any
>> additional memory cost (other than potentially increasing the size of some
>> symbol names), and provide an easy way to catch (and unique) collisions.
>> Though for correctness, the variables should only be renamed if they are
>> internal. In theory, this is currently broken because you could create a
>> symbol ".foo" at the IR level and try to get its address using the CUDA
>> driver API, but fail since the symbol name would really be _$_foo in the
>> binary. Perhaps we could loudly fail if an externally visible symbol would
>> need mangling/uniquing, and just mangle/unique the symbol if its internal.
> I opened PR19099 to keep track of this. A pre-codegen IR pass makes sense
> to me. However, what about external symbols? In theory, they are useless if
> ptxas can't assemble them anyway.
Right, if a symbol is internal, we can mangle/unique it. But if its
externally-defined or externally-visible, we can't do anything to it. It's
not perfect, but its likely the best we can do given the current
constraints in the PTX assembler.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits