[patch] Assert that clang's datalayout strings are in sync with llvm's

[Alp Toker]

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