[llvm] r178416 - [NVPTX] Add NVVMReflect pass to allow compile-time selection of

[Benjamin Kramer]

On 30.03.2013, at 17:33, Justin Holewinski <justin.holewinski at gmail.com> wrote:

> On Sat, Mar 30, 2013 at 11:58 AM, Sean Silva <silvas at purdue.edu> wrote:
> Could you give a bit more background on the motivation for this? I wasn't able to find any discussion on llvmdev.
> 
> The only real motivation is compatibility with CUDA.  This pass is needed for linking with certain CUDA libraries that rely on this to implement selecting between code paths based on fast-math flags.  I would love to get rid of this in the future, but it was created long before I had any say in it. :)
> 
> The idea is that most users won't use it, but those that want to link to certain CUDA libraries will (at least for now).  I just want to upstream it to maximize compatibility between the open-source version and the proprietary bits that ship with CUDA.
>  
> 
> +// This pass replaces occurences of __nvvm_reflect("string") with an
> +// integer based on -nvvm-reflect-list string=<int> option given to this pass.
> 
> Please give an example of usage here.
> 
> It also seems that in some places you say "0/1", but in this description you say "<int>". Is any int allowed? Please clarify.
> 
> Technically any int is allowed, but only 0/1 is actually used in practice right now.  I'll update the comments.
>  
> 
> +    int reflectVal = 0; // The default value is 0
> 
> Please put this information in the top-of-the-file comment.
> 
> Sure.
>  
> 
> +  for (unsigned i = 0, e = ReflectList.size(); i != e; ++i) {
> +    //    DEBUG(dbgs() << "Option : "  << ReflectList[i] << std::endl);
> 
> +    reflectArg = reflectArg.substr(0, reflectArg.size() - 1);
> +    //    DEBUG(dbgs() << "Arg of _reflect : " << reflectArg << std::endl);
> 
> +  //std::map<std::string, int> VarMap;
> 
> +  StringMap<int> VarMap;
> 
> Please remove commented-out code.
> 
> Oops...
>  
> 
> +static std::vector<std::string>
> +Tokenize(const std::string &str, const std::string &delim) {
> +  std::vector<std::string> tokens;
> 
> Is there a particular reason you aren't following the LLVM naming convention here and throughout?
> 
> Not really.  Internally, attempts to follow the LLVM style are lax.  This is just a case I missed... :(

For this function there's StringRef::split (with a SmallVector argument) which does the same thing.

- Ben

> 
> Thanks for the review!
>  
> 
> -- Sean Silva
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
> 
> -- 
> 
> Thanks,
> 
> Justin Holewinski
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits