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