[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

Tue Apr 20 15:39:10 PDT 2021

echristo added a comment.

In D100776#2699603 <https://reviews.llvm.org/D100776#2699603>, @thakis wrote:

> In D100776#2699565 <https://reviews.llvm.org/D100776#2699565>, @rnk wrote:
>
>> Of the three people who commented on D17183 <https://reviews.llvm.org/D17183>, you and I are on the only ones in favor of this approach. I think @echristo and @jyknight both preferred approach 2. I'd like to get at least an agreement to disagree from one of them before approving this.
>
> That's 50% in people, and 15/24 voting by changes that landed in clang/ since start of year ;) Don't know why 2/4 is "only".

I mean that's fair ;)

> I'm also not dismissing approach 2: I implemented it as well, and it's ready-to-go as soon as someone wants to pursue the direction mentioned there. If someone wants to do that, I don't have a problem with it, but until then this here is simpler and more self-consistent since it preserves the original design of TargetInfo.

how about some historical context. I'm reasonably certain I was one of the motivators behind this change. The prior state often lead to either very subtle bugs that were hard to diagnose without an asserts build or a fairly inscrutable assert that the front and back ends disagreed on what context a string should have. The idea was that you could then ask the "backend" how to construct up a layout for what was wanted.

As is mentioned there are tradeoffs around this though: a) it does make it harder to have clang generate code without a backend or llvm itself around, b) it does have a dependency when none existed.

So, if this is really causing some consternation then we can pull back and reinstate what we had, but it was a direction around solving a set of hard to find bugs.

Thoughts?

-eric

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

https://reviews.llvm.org/D100776