Thanks!

Thanks for the unit test!

The change looks reasonable to me.

The local change itself looks now good to me, but @arsenm may have general concerns with the approach here, so please address his comment first.

LGTM, one minor inline comment.

LGTM -- one inline comment on style.

I would not be so worried about the complexity of having to maintain multiple version formats -- we can think of the version flag as setting defaults for command-line options that alter the test format.
We already have such options (e.g. mentioned --function-signature), so this is not new in principle, it just provides a convenient means to set these.

Thanks for the review, I've updated the patch accordingly.

Restrict alignment check to integer types as suggested by review.

Thanks for the review.
Your example of accessing a known pointer in a vector is interesting.

Thanks for the suggestion, that is much cleaner indeed. I updated the patch accordingly.

Review feedback: Simplify testcase by passing DL to opt

Abandoning this change in favor of avoiding GEPs into vectors instead of fixing their offsets.

Abandoning, we will require i8 to be naturally aligned in the datalayout as discussed on discourse. Sorry for leaving it open for so long.

Thanks for the refactoring. One more thought inline.

Thanks for reviewing @nikic. Comments inline.

Address review feedback: Handle invalid data layouts gracefully

High-level comment regarding how to pass callbacks to parser functions: We have quite a few different parser routines, both for textual IR and bitcode, and special modes (e.g. lazy loaders).
This leads to many places where we would add essentially the same arguments, often with default values. Some functions miss these (e.g. getLazyBitcodeModule), because nobody bothered so far to add callback(s) for them.

In D140985#4028591, @arsenm wrote:

Is DataLayout compatibility still determined with string equality and not semantic equality?

For context, this prepares requiring naturally aligned i8 as discussed in https://discourse.llvm.org/t/status-of-overaligned-i8/66913/

Add lit test case.

Ok, fine with me.

I still get the ${CONFIGURATION} directory even with this patch, running:

In D139973#4004344, @barannikov88 wrote:

• std::any uses RTTI, so it is unlikely to ever replace llvm::Any.

LGTM, but let's wait for the other reviewers.

In D133379#3971623, @beanz wrote:

In D133379#3962893, @jsilvanus wrote:

How should this behave on overaligned element types? For example, with a i16:32 data layout (as in the old DXIL data layout),
the second element in <3 x i16> will not be 32-bit aligned, even if the vector is.

16-bit types aren't supported with the old data layout at all. If you enable 16-bit types when compiling HLSL you get the new data layout which aligns 16-bit types to 16-bit boundaries. The only exception to that is some oddities around how buffers are packed, but that doesn't impact the data layout.

Thanks for the review.
Your idea of adding a gep_offset_iterator seems plausible. I'll look into that.

Adding reviewers, based on past changes to GetElementPtrTypeIterator.h.
Please feel free to reject or suggest other reviewers that might have a better background.

This alters the vector ABI alignment so that vector elements are always aligned appropriately for their type.

This is the patch for the mentioned issue of GEPs into vectors of overaligned elements: https://reviews.llvm.org/D139034

I've started a discourse discussion on the topic: https://discourse.llvm.org/t/status-of-overaligned-i8/66913

I don't need support for overaligned i8s, it is fine for me to have LLVM require natural alignment on i8.

Yes, you are right, I miscalculated by one bit :) Updated the test accordingly.

Fix constant in test case.

Add test case that triggers the assert.

I couldn't spot any nontrivial difference in generated code -- so the new namespace-based info is equally useful (or useless?) for AA?

In these places, we try to obtain a pointer based on a base pointer and a byte offset, and do so using a GEP on i8, using the offset as index.
This will fail if the AllocSize of i8 is not 1. The added assertions detect such cases instead of generating wrong code.

Are you interested in a lit test checking that the assertion fires? Are we testing individual asserts this way?

Not your code, but:

Thanks for your review, I updated the patch to address it.

Remove reformatting of existing code.

Address review feedback.

Hi David, thanks for looking into this change!

Thanks for reviewing.

Fix branch target.

LGTM, thanks!

Thanks, I also have a separate possible use case for this. Two minor inline comments.

Thanks for the insights Nicolai and Jay!

One more thought on longer expressions: Nicolai already mentioned that we currently only match a particular expression structure.
I wondered whether the expression structure has any performance impact?

I see, thanks!

As such case cannot be combined into a BFI instruction, I guess it's relatively straight-forwardly translated into something like this:

One inline comment on a comment :).

Implement review feedback:

Thanks for the review! I've implemented all your suggestions, except for the deduction guide, cf. the inline comment.

Implement review feedback.