[cfe-dev] [PATCH] Driver modifications for cross compilation

Fri May 27 10:09:57 PDT 2011

2011/5/26 James Molloy <mankeyrabbit at gmail.com>

> Hi Matthieu,
>
> Barge away!
>
> Pointers were used as the value type as several functions return a pointer
> deliberately so they can indicate failure with NULL. The same could be
> achieved using references by returning a sentinel value or having a bool&
> argument, but the Archs and Cpus must reference each other, and there is no
> safe way to get a T* from a map of ValueTy T& - the lifetime of the
> reference is undefined.
>
> Unless there is some guarantee in std::map that I am unaware of?
>
>
std::map is a node-based container: once a node has been introduced in it,
and until it is explicitly "erased", it will always stay at the same memory
address. References and pointers are guaranteed not to go stale because of
insert or erase (other than the node). Using the  map[id] = X;  will replace
the value in place (using assignment). You should not have any issue
removing the "*" in "std::map<llvm::StringRef, ArchDef*>" therefore.

For Archs and Cpus referencing each other, indeed the pointer seems the
right approach. There is no memory ownership involved here anyway.

> Regarding extra lookups- You're quite right - I should factor them to one
> lookup per insert. As these functions are only called once I didn't pay too
> much attention to their runtime cost.
>
> Did you have any other comments before I update my patch with your
> suggested change?
>
> Cheers,
>
> James
>
>
-- Matthieu

>
> On 26 May 2011, at 18:35, Matthieu Monrocq <matthieu.monrocq at gmail.com>
> wrote:
>
>
> Message: 4
>> Date: Thu, 26 May 2011 13:13:10 +0100
>> From: "James Molloy" < <james.molloy at arm.com>james.molloy at arm.com>
>> Subject: [cfe-dev] [PATCH] Driver modifications for cross compilation
>> To: < <cfe-dev at cs.uiuc.edu>cfe-dev at cs.uiuc.edu>
>> Message-ID: <000401cc1b9e$4796f540$d6c4dfc0$@ <molloy at arm.com>
>> molloy at arm.com>
>> Content-Type: text/plain; charset="windows-1252"
>>
>> Hi,
>>
>>
>>
>> I noticed that the Clang driver requires the user to set
>> "-ccc-host-triple"
>> for cross-compilation (if not using the -arch behaviour on Darwin). I
>> thought this was a little strange as the option is not documented in the
>> --help, is nonstandard and nonobvious.
>>
>>
>>
>> I delved a little deeper, and found a bunch of FIXMEs in the driver
>> wanting
>> to tablegen lots of if/else statements selecting specific architecture
>> options (AddARMTargetArgs being the worst culprit). Adding to that, some
>> of
>> these monolithic functions were duplicated with static linkage in multiple
>> files.
>>
>>
>>
>> I've factored most of these if/else blocks into one tablegen file, which
>> when #included forms an "architecture definition database" from which one
>> can query default CPUs for given architecture/OS, which architecture a CPU
>> belongs to, and any other target-specific feature of the CPU defined in
>> the
>> .td file.
>>
>>
>>
>> The tablegen file is a list of known architecture variants and CPUs for
>> different platforms; we are happy to maintain the ARM part of this.
>>
>>
>>
>> The upshot of this is that several monolithic functions have disappeared:
>>
>>  * Tool{s,Chain}.cpp:GetARMTargetCPU          (replaced with
>> Arch->DefaultCpu)
>>
>>  * Tool{s,Chain}.cpp:getLLVMArchSuffixForARM  (replaced with
>> Arch->Properties["LLVMArchSuffix"])
>>
>>
>>
>> And "Tools.cpp:Add{ARM,Sparc,MIPS,X86}TargetArgs" have been simplified to
>> contain fewer if/elses.
>>
>>
>>
>> The above was all just general tidyup; after this refactor, a 3 line
>> change
>> in Driver.cpp now causes autodetection of the host triple from any -mcpu=
>> or
>> -march= options passed. For example:
>>
>>
>>
>>    clang -mcpu=cortex-a8 test.c -o test.o -c
>>
>>
>>
>> will set the HostTriple to "armv7--", which obviously then enables
>> AddARMTargetArgs to be called instead of AddX86TargetArgs, and correct ARM
>> code is produced.
>>
>>
>>
>> I feel that this is a much more sane behaviour than mandating the use of
>> -ccc-host-triple (although the behaviour of that option has not been
>> altered
>> and overrides any auto-detection); if a user specifies a CPU which is
>> obviously part of an architecture, or a
>> -march=something-that-isn't-the-host, the HostTriple should be detected
>> automatically.
>>
>>
>>
>> As a result of this, taking the line of thought that -ccc-host-triple
>> should
>> be optional and should be able to be fully functionally replaced by
>> -mcpu=/-march= and friends, I realised there was no way to dictate the
>> host
>> OS without use of -ccc-host-triple. So I added a -mos= option, which sets
>> the OSAndEnvironment part of the llvm::Triple. This way, almost all
>> functionality offered by overriding the triple (still wanted for power
>> users) is available via more recognisable command line options.
>>
>>
>>
>> The patch I have attached is the entire patch + testcases, so is rather
>> large. If accepted, I intend to send to the commit list in several chunks:
>>
>>
>>
>>  1. Tablegen file, new files ArchDefs.h and ArchDefs.cpp   (+ new tablegen
>> backend to llvm-commits)
>>
>>       - This patch should not alter the behaviour of Clang at all, hence
>> no
>> tests being added.
>>
>>  2. First usecase of the ArchDefs class: the ARM target. Removes
>> GetARMTargetCPU and getLLVMArchSuffixForARM, and changes the default
>> command
>> line behaviour to infer target triple from -mcpu/-march if present.
>>
>>       - As there is now a use case for the ArchDefs class, testcases will
>> be added to this patch.
>>
>>  3. Refactoring intel / mips target specifics to use ArchDefs. This is the
>> most likely patch to cause problems with other uses that I haven't
>> forseen.
>>
>>  4. Adding -mos= and making it have an effect on the host triple.
>>
>>
>>
>> Patch 2. also contains several bugfixes to the ARM driver, namely:
>>
>>  * ARM target driver would not detect -mabi=eabi/gnueabi correctly.
>>
>>  * ARM target driver when setting -mabi= would not update the Triple and
>> so
>> the float-abi check later would be incorrect.
>>
>>  * ARM target driver did not have knowledge of several ARM cores:
>> Cortex-A{8,9,15}, Cortex-R{4,5,7}, Cortex-M{0,3,4}, ARM11MPCore, ARM1176.
>>
>>
>>
>> Review of both the patch and the intended functional change would be
>> excellent.
>>
>>
>>
>> Cheers,
>>
>>
>>
>> James
>> -------------- next part --------------
>> An HTML attachment was scrubbed...
>> URL:
>> <http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20110526/e2d1ac2d/attachment.html>
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20110526/e2d1ac2d/attachment.html
>> -------------- next part --------------
>> A non-text attachment was scrubbed...
>> Name: clang-all.patch
>> Type: application/octet-stream
>> Size: 38709 bytes
>> Desc: not available
>> Url :
>> <http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20110526/e2d1ac2d/attachment.obj>
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20110526/e2d1ac2d/attachment.obj
>> -------------- next part --------------
>> A non-text attachment was scrubbed...
>> Name: tablegen-clang-gen-arch-defs.patch
>> Type: application/octet-stream
>> Size: 7289 bytes
>> Desc: not available
>> Url :
>> <http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20110526/e2d1ac2d/attachment-0001.obj>
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20110526/e2d1ac2d/attachment-0001.obj
>>
>> ------------------------------
>>
>> _______________________________________________
>> cfe-dev mailing list
>>  <cfe-dev at cs.uiuc.edu>cfe-dev at cs.uiuc.edu
>>  <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>> End of cfe-dev Digest, Vol 47, Issue 83
>> ***************************************
>>
>
> Hello,
>
> Sorry to barge in, but I found the subject interesting so I delved into the
> patch, and I have a couple of comments.
>
> If I may:
>
> +        typedef std::map<llvm::StringRef, ArchDef*> StringArchMap;
> +        typedef std::map<llvm::StringRef, CpuDef*>  StringCpuMap;
>
> Why use pointers here ? Plain values would avoid leaks.
>
>
> Continuing:
>
> + if(!Archs[id]) Archs[id] = new ArchDef;
> + Archs[id]->ArchID = id;
> + Archs[id]->OS = os;
> + Archs[id]->Name = name;
> + Archs[id]->DefaultCpuName = defaultcpuname;
>
> This involves a lot of lookups (which in turns involve a lot of calls to strcmp), the equivalent could be achieved with:
>
> ArchDef *& A = Archs[id]; // default constructed value is 0
> if (!A) { A = new ArchDef; }
> A->ArchiID = id;
> A->OS = os;
> A->Name = name;
> A->DefaultCpuName = defaultcpuname;
>
> If overwriting is not a feature, then the whole lot should be wrapped into
> the block of the  if
>
>
> Cheers,
> -- Matthieu
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110527/39f9fb49/attachment.html>