On 20/12/2013 16:55, Rafael Espíndola wrote:
The idea is fantastic, but I found your implementation burdensome.
Bringing a TargetMachine dependency into IRGen, even if it's a "soft"
dependency is best avoided because the clear layering between IRGen and
CodeGen is LLVM's stated foremost design goal.
The extraction of CreateTargetMachine() into a long list of parameters in
your patch also left something to be desired and we should be able to avoid
it.
It might be possible to still refactor things to avoid the
duplication, but for now this should at least make sure both projects
stay in sync.
I've attached my take on your idea, implementing it in a net 8 LoC.
In my patch, the verification is done at IRGen so there's no runtime
penalty, and moreover no change in layering.
Interesting. I started coding with the idea of removing the string
from clang and using the ones in llvm at their current location. I got
there and then went back to just asserting, but never looked back if
there was a better spot to put the assert.
I really like your patch. Some some small comments.
* Making TM a member of EmitAssemblyHelper is a nice independent
improvement. Please just commit that right away :-)
+ if (!TM) TM.reset(CreateTargetMachine(UsesCodeGen));
Using two lines seems to be the more current stile (what clang-format uses).
Thanks, cleanup part landed in r197832 along with the tweak.
Alp.
+ assert(TDesc.empty() || !AsmHelper.TM ||
This is failing on test/Frontend/ir-support-codegen.ll. It seems like
the assert found a real bug. The TargetInfo is created once with
// Create the target instance.
setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), &getTargetOpts()));
but then CodeGenAction will simply parse the module and use the triple in it.
Given what a corner case this is, it might be reasonable to just pass
an empty string in CodeGenAction.cpp:424 or maybe patch the Triple in
the module. After all, clang always creates a single TargetInfo based
on what is passed to -triple (or the system default), in cannot really
handle an IR file with a different one.
Even with this verification done, we should continue to investigate options
for sharing the layouts and getting them threaded through the frontend. Two
copies are one too many.
Agreed. I just don't promise I will have the cycles to do it right
now. The assertion gives me enough confidence to add the features I
need to DataLayout.
Cheers,
Rafael
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
