Page MenuHomePhabricator

[OpenMP] Clean up variable and function names for NVPTX backend
AbandonedPublic

Authored by gtbercea on Oct 17 2017, 8:27 AM.

Details

Summary

Clean-up variable and function names.

Diff Detail

Repository
rL LLVM

Event Timeline

gtbercea created this revision.Oct 17 2017, 8:27 AM
tra accepted this revision.Oct 17 2017, 9:18 AM
This revision is now accepted and ready to land.Oct 17 2017, 9:18 AM
jlebar added a subscriber: jlebar.Oct 17 2017, 10:46 AM

This has been tried twice before, see D29883 and D17738. I'm as unhappy about this as anyone, and personally I don't have any preference about how we try to solve it. But I think we shouldn't check this in without hearing the objections from those past attempts.

jlebar requested changes to this revision.Oct 17 2017, 10:46 AM
This revision now requires changes to proceed.Oct 17 2017, 10:46 AM
tra requested changes to this revision.Oct 17 2017, 11:03 AM

Justin is right. I completely forgot about this. :-/
Hal offered possible solution: https://reviews.llvm.org/D17738#661115

Hi Artem, Justin,

I see that this patch is the same as the patch Arpith wanted to post a while back i.e. D17738.

Was there a consensus regarding what the right thing to do is in this case?

I'd be interested to get the ball rolling in regard to coming up with a fix for this. I see some suggestions in past patches. Some help/clarification would be much appreciated.

Thanks!

I'd be interested to get the ball rolling in regard to coming up with a fix for this. I see some suggestions in past patches. Some help/clarification would be much appreciated.

Happy to help, but I'm not sure what to offer beyond the link in Art's previous comment.

I'd be interested to get the ball rolling in regard to coming up with a fix for this. I see some suggestions in past patches. Some help/clarification would be much appreciated.

Happy to help, but I'm not sure what to offer beyond the link in Art's previous comment.

Thanks Justin. Perhaps if we could start by clarifying this statement "One option is that we add a function to LLVM get an available separator character, which can default to '.', but we set to '$' for nvptx, and use that for generating new names at the IR level." as well as this statement "This seems practical. Perhaps it could be part of the name mangling scheme already encoded in DataLayout?".

The first question that comes to mind is what is the link between data layout and name mangling conventions?

The first question that comes to mind is what is the link between data layout and name mangling conventions?

I pulled up http://llvm.org/doxygen/classllvm_1_1DataLayout.html and searched for "mangling" -- presumably this is what they were referring to. We also don't need to speculate, rnk still works on LLVM. :)

hfinkel edited edge metadata.Oct 23 2017, 5:49 PM

The first question that comes to mind is what is the link between data layout and name mangling conventions?

I pulled up http://llvm.org/doxygen/classllvm_1_1DataLayout.html and searched for "mangling" -- presumably this is what they were referring to. We also don't need to speculate, rnk still works on LLVM. :)

DataLayout generally holds information that the target-independent optimizer needs in order to simplify the IR into our canonical form. This is as opposed to TargetTransformInfo, which provides data necessary to optimize the IR in target-aware ways (e.g., do things that are orthogonal to canonicalization such as inlining and vectorization). It is also as opposed to external utility functions that might be used by the frontend (e.g., llvm::sys::getHostCPUName()). If I recall correctly, this is information that would be used by the frontend when generating the IR, and the function results are controlled by the triple. As a result, I think that a general utility function somewhere would be fine.

Hahnfeld edited edge metadata.

Fixed differently in rL319657.

gtbercea abandoned this revision.Feb 8 2018, 8:05 AM