[llvm-commits] [PATCH] LTO code generator options
Daniel Dunbar
daniel at zuster.org
Tue Nov 24 15:41:56 PST 2009
Hi Viktor,
--
> Index: include/llvm/ADT/Triple.h
> ===================================================================
> --- include/llvm/ADT/Triple.h (revision 89702)
> +++ include/llvm/ADT/Triple.h (working copy)
> @@ -131,6 +131,18 @@
> }
>
> /// @}
> + /// @name Operators
> + /// @{
> +
> + /// operator =(StringRef) - Set all components to the new triple
> + /// from the given string representation. Used in the cl::opt parser.
> + Triple& operator =(const StringRef Str) {
> + Data = Str;
> + Arch = InvalidArch;
> + return *this;
> + }
I don't think this should be necessary, I would rather have an
explicit Triple call, or if that is too painful just make the Triple()
constructor (which takes a StringRef) implicit.
Note that the 'const' on Str isn't needed.
> +
> + /// @}
> /// @name Typed Component Access
> /// @{
>
> Index: tools/lto/LTOCodeGenerator.h
> ===================================================================
> --- tools/lto/LTOCodeGenerator.h (revision 89702)
> +++ tools/lto/LTOCodeGenerator.h (working copy)
> @@ -19,6 +19,7 @@
> #include "llvm/LLVMContext.h"
> #include "llvm/ADT/StringMap.h"
> #include "llvm/ADT/SmallVector.h"
> +#include "llvm/ADT/Triple.h"
>
> #include <string>
>
> @@ -50,12 +51,14 @@
> const std::string& objPath, std::string& errMsg);
> void applyScopeRestrictions();
> bool determineTarget(std::string& errMsg);
> + void parseCommandLineOptions();
>
> typedef llvm::StringMap<uint8_t> StringSet;
>
> llvm::LLVMContext& _context;
> llvm::Linker _linker;
> llvm::TargetMachine* _target;
> + llvm::Triple _targetTriple;
I don't think _targetTriple needs to be added here, I think it can be
local to the function that uses it. If it does need to be added to the
class, I would prefer having a function like getTriple() which
computed, cached, and returned the triple, instead of having the
member variable mutated inside some other function.
> bool _emitDwarfDebugInfo;
> bool _scopeRestrictionsDone;
> lto_codegen_model _codeModel;
> Index: tools/lto/LTOCodeGenerator.cpp
> ===================================================================
> --- tools/lto/LTOCodeGenerator.cpp (revision 89702)
> +++ tools/lto/LTOCodeGenerator.cpp (working copy)
> @@ -59,7 +59,29 @@
> static cl::opt<bool> DisableInline("disable-inlining",
> cl::desc("Do not run the inliner pass"));
>
> +static cl::opt<Triple, false, cl::parser<std::string> > TargetTriple("mtriple",
> + cl::desc("Override target triple for module (see -version for available targets)"),
> + cl::value_desc("<arch>-<vendor>-<os>[-<environ>]"));
> +
> +static cl::opt<std::string> MCPU("mcpu",
> + cl::desc("Override a target specific cpu type (see -mattr=help for available CPUs)"),
> + cl::value_desc("cpu-name"),
> + cl::init(""));
>
> +// Ignore -mtune option which could be defined by gcc.
> +// We don't use it, just don't want to get unrecognized parameter error.
> +static cl::opt<std::string> MTune("mtune",
> + cl::desc("Override a target specific cpu type (see -mattr=help for available CPUs)"),
> + cl::value_desc("cpu-name"),
> + cl::init(""), cl::Hidden);
> +
> +static cl::list<std::string> MAttrs("mattr",
> + cl::CommaSeparated,
> + cl::desc("Target specific attributes (see -mattr=help for details)"),
> + cl::value_desc("a1,+a2,-a3,..."));
> +
> +static bool opt_parsed = false;
> +
While it isn't new in this patch, the use of llvm::cl here is really
not good, and I'm nervous about increasing our dependency on it. Among
other things, this is making it harder to use it in a threaded
context.
> const char* LTOCodeGenerator::getVersionString()
> {
> #ifdef LLVM_VERSION_INFO
> @@ -140,6 +162,12 @@
>
> bool LTOCodeGenerator::writeMergedModules(const char *path,
> std::string &errMsg) {
> + // This method is exposed by the LTO API, it starts code generation
> + // process and could be called from the outside.
> + // So we need to parse the code generator command line options here.
> + parseCommandLineOptions();
> + // Determine and create a target platform object for
> + // the processing data.
> if (determineTarget(errMsg))
> return true;
Newline before new //?
>
> @@ -171,6 +199,15 @@
>
> const void* LTOCodeGenerator::compile(size_t* length, std::string& errMsg)
> {
> + // This method is exposed by the LTO API, it starts code generation
> + // process and could be called from the outside.
> + // So we need to parse the code generator command line options here.
> + parseCommandLineOptions();
> + // Determine and create a target platform object for
> + // the processing data.
> + if (determineTarget(errMsg))
> + return NULL;
Newline before new //?
> +
> // make unique temp .s file to put generated assembly code
> sys::Path uniqueAsmPath("lto-llvm.s");
> if ( uniqueAsmPath.createTemporaryFileOnDisk(true, &errMsg) )
> @@ -227,6 +264,9 @@
> bool LTOCodeGenerator::assemble(const std::string& asmPath,
> const std::string& objPath, std::string& errMsg)
> {
> + // Target must be set at this point
> + assert(_target);
> +
Please use:
--
assert(_target && "Target must be set at this point!");
--
instead, which is in keeping with LLVM style.
Actually, what would be better would be to just add a function
--
TargetMachine &getTarget()
--
which returns and caches the target, and don't use _target directly.
This reduces dependencies in the code.
> sys::Path tool;
> bool needsCompilerOptions = true;
> if ( _assemblerPath ) {
> @@ -281,12 +321,20 @@
> bool LTOCodeGenerator::determineTarget(std::string& errMsg)
> {
> if ( _target == NULL ) {
> - std::string Triple = _linker.getModule()->getTargetTriple();
> - if (Triple.empty())
> - Triple = sys::getHostTriple();
> + // Use the tripple if the option was explicitly set
tripple -> triple
> + _targetTriple = TargetTriple;
> + // Or take it from the bitcode module if the option wasn't set explicity
> + if (_targetTriple.getTriple().empty())
> + _targetTriple = llvm::Triple(_linker.getModule()->getTargetTriple().c_str()) ;
> + // Still empty? Use the host triple as the last choise.
choise -> choice and more newlines before comments.
> + if (_targetTriple.getTriple().empty())
> + _targetTriple = Triple(sys::getHostTriple());
>
> + // At this point triple string should not be empty.
> + assert(!_targetTriple.getTriple().empty());
Again, please put the assert "reason" inside the assert instead of in a comment.
> +
> // create target machine from info for merged modules
> - const Target *march = TargetRegistry::lookupTarget(Triple, errMsg);
> + const Target *march = TargetRegistry::lookupTarget(_targetTriple.getTriple(), errMsg);
> if ( march == NULL )
> return true;
>
> @@ -308,13 +356,27 @@
> SubtargetFeatures features;
>
> // Set the rest of features by default.
> - // Note: Please keep this after all explict feature settings to make sure
> - // defaults will not override explicitly set options.
> - features.AddFeatures(
> - SubtargetFeatures::getDefaultSubtargetFeatures(llvm::Triple(Triple)));
> + // Note: Please keep this before all explict feature settings to make sure
> + // defaults will be overrided by explicitly set options.
Please rephrase to just be an explanation, something like:
// Initialize the target features starting from the default and applying the
// explicitly set options which may override the defaults.
is more readable IMHO.
> + std::pair<StringRef, StringRef> pair =
> + StringRef(SubtargetFeatures::getDefaultSubtargetFeatures(_targetTriple)).
> + split(",");
> + do {
> + features.AddFeature(pair.first);
> + pair = pair.second.split(",");
> + } while (pair.first.size() > 0);
This assumes getDefaultSubtargetFeatures returns a non-empty string.
>
> + if (!MCPU.empty())
> + features.setCPU(MCPU);
> +
> + if (!MAttrs.empty()) {
> + for (unsigned i = 0; i != MAttrs.size(); ++i)
> + features.AddFeature(MAttrs[i]);
> + }
> +
> // construct LTModule, hand over ownership of module and target
> - _target = march->createTargetMachine(Triple, features.getString());
> + _target = march->createTargetMachine(_targetTriple.getTriple(),
> + features.getString());
> }
> return false;
> }
> @@ -358,8 +420,8 @@
> bool LTOCodeGenerator::generateAssemblyCode(formatted_raw_ostream& out,
> std::string& errMsg)
> {
> - if ( this->determineTarget(errMsg) )
> - return true;
> + // Target must be set at this point.
> + assert(_target);
See prev comments.
> // mark which symbols can not be internalized
> this->applyScopeRestrictions();
> @@ -380,11 +442,6 @@
> assert (0 && "Unknown exception handling model!");
> }
>
> - // if options were requested, set them
> - if ( !_codegenOptions.empty() )
> - cl::ParseCommandLineOptions(_codegenOptions.size(),
> - (char**)&_codegenOptions[0]);
> -
> // Instantiate the pass manager to organize the passes.
> PassManager passes;
>
> @@ -459,3 +516,16 @@
> _codegenOptions.push_back(strdup(o.c_str()));
> }
> }
> +
> +/// Parse the command line and set the code generator options.
> +void LTOCodeGenerator::parseCommandLineOptions()
> +{
> + // if options were requested, set them once.
> + if ( !opt_parsed && !_codegenOptions.empty() ) {
> + cl::ParseCommandLineOptions(_codegenOptions.size(),
> + (char**)&_codegenOptions[0]);
> + opt_parsed = true;
> + }
> +}
> +
> +
> Index: tools/lto/LTOModule.cpp
> ===================================================================
> --- tools/lto/LTOModule.cpp (revision 89702)
> +++ tools/lto/LTOModule.cpp (working copy)
> @@ -131,19 +131,23 @@
> if (!m)
> return NULL;
>
> - std::string Triple = m->getTargetTriple();
> - if (Triple.empty())
> - Triple = sys::getHostTriple();
> + // Target overriding for a single module is not supported.
> + // Use host triple as default if target is not defined in the module.
> + Triple targetTriple(m->getTargetTriple());
> + if (targetTriple.getTriple().empty())
> + targetTriple.setTriple(sys::getHostTriple());
>
> // find machine architecture for this module
> - const Target* march = TargetRegistry::lookupTarget(Triple, errMsg);
> + const Target* march =
> + TargetRegistry::lookupTarget(targetTriple.getTriple(), errMsg);
> if (!march)
> return NULL;
>
> // construct LTModule, hand over ownership of module and target
> - const std::string FeatureStr =
> - SubtargetFeatures::getDefaultSubtargetFeatures(llvm::Triple(Triple));
> - TargetMachine* target = march->createTargetMachine(Triple, FeatureStr);
> + const std::string featureStr =
> + SubtargetFeatures::getDefaultSubtargetFeatures(targetTriple);
> + TargetMachine* target =
> + march->createTargetMachine(targetTriple.getTriple(), featureStr);
> return new LTOModule(m.take(), target);
> }
>
--
- Daniel
On Mon, Nov 23, 2009 at 7:38 PM, Viktor Kutuzov
<vkutuzov at accesssoftek.com> wrote:
> The updated patch is attahced. It reflects the changess Chris and Daniel has
> requested.
>
> Best regards,
> Viktor
>
> ----- Original Message ----- From: "Rafael Espindola" <espindola at google.com>
> To: "Viktor Kutuzov" <vkutuzov at accesssoftek.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, November 23, 2009 1:09 PM
> Subject: Re: [llvm-commits] [PATCH] LTO code generator options
>
>
>>> Thanks a lot for reviewing the patch.
>>> It is commited as http://llvm.org/viewvc/llvm-project?rev=89516&view=rev
>>>
>>> Now everything is reaady for the target triple overriding.
>>
>> I should be able to take a look at it tomorrow, but it would help if
>> you could first implement Daniel's and Chirs' comments.
>>
>>>
>>> Best regards,
>>> Viktor
>>
>> Cheers,
>> --
>> Rafael Ávila de Espíndola
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
More information about the llvm-commits
mailing list