[PATCH] D17183: Make TargetInfo store an actual DataLayout instead of a string.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Zombie comment time! I kind of don't love this change. clang's libBasic shouldn't depend on llvm/lib/IR: Practically there's no reason that building clang-format should build lib/IR and its deps, and more importantly semantically the dependency feels off too.

The dep from libBasic to lib/IR was added in https://reviews.llvm.org/rG8d043e82ef1179fed1d3d85171905c55bda7312f (oct 2014) then one day later http://reviews.llvm.org/rG86204b21e98b9d73b9ab381c87862837d2357082 formalized that but https://reviews.llvm.org/rGa0ac3c2bf049eb28e77a19dd70cbd570585f4747 (two days later, also oct 2014) removed the dependency again. Alas, not from the CMakeLists.txt. Then, 1.5 years later, this added it back, and it's still here. I'd like to remove the dependency from clangBasic on lib/IR again.

I see these options:

1. Kind of revert this patch. Make TargetInfo again store the datalayout str and the prefix, and then assert somewhere that sees both layers (CodeGen) that the prefix matches the mangling in the layout string. Practically, I'd give resetDataLayout() the prefix as 2nd arg since that seems like a good place to set both. This abstractly makes sense to me since TargetInfo for the most part duplicates DataLayout.

2. split llvm/IR/DataLayout into DataLayoutString that just does the parsing and pointer width and primitive things and move that part to llvm/Support, and make IR/DataLayout use that new llvm/Support/DataLayoutString to do the llvm::Type-dependent bits. That way, TargetInfo could lean _more_ on the DataLayoutString for pointer sizes and widths and whatnot. But this general approach leads to everything being in llvm/Support over time :)

3. (not sure if feasible) move TargetInfo stuff out of Basic into a new libTargetInfo. Then the dep on IR could stay. That fixes layering but doesn't address that TargetInfo is independent of DataLayout except for this one thing.

I lean towards (1) since that feels like it's most in the spirit of the original TargetInfo design.

Does anyone have strong opinions?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D17183/new/

https://reviews.llvm.org/D17183