But it was useful to have it separate at least for review purposes, since it made it much easier when Michael asked me to proofread the change from 'flags' to 'tags'!

Ping? @lenary agreed that this is the least bad option to fix the codegen fault, and a week ago I fixed the only problem I knew of with the patch.

I think that most ideally, if you don't know the true address of the instruction being disassembled, the best way to disassemble ADR is to indicate explicitly that the address you're going to end up with depends on that unknown, and not make it look as if it's a constant.

A much needed improvement – thanks!

Revised the patch to make the right decision on Armv6 and before, where Arm and Thumb-1 jump tables are both available, and we always want to pick Arm because it's smaller and faster. This also exposed a bug in which `getJumpTableEntrySize was ignoring the fact that selectJumpTableArmEncoding` might have swapped to the other one of Arm and Thumb (and until now it didn't matter because the table entries were always the same size for both).

Nevermind, this doesn't actually help what you're doing; the Thumbv6m thunks are a different size.

I see what you mean, but in that case, how do we deal with the fact that there are multiple TargetTransformInfos involved? If there was just one for the whole module then that would make sense, but there's one per function. In the current version of the patch I'm iterating over all of them asking about b.w support, and making a single decision about the jump table format based on the results of all those queries.

I don't see how. TargetTransformInfo isn't a base class with target-specific descendants. It wraps an Impl class which you can't get out of it, and even if you could, there's no LLVM-style dyn_cast setup to allow detecting its concrete type.

LGTM, with a small commenting nit.

Added draft release notes, and an extra comment in the modified test file.

"Make difficult things possible": perhaps it might be useful to make sure it's at least possible to express a complex boolean function of the basic predicates, even if it's cumbersome? (So that, for example, you could make an attribute conditional on "this cc1 option but not that one".) I assume YAML would let you write something like an expression AST in hierarchical form, with predicates like regex: at the leaves, and internal nodes for AND, OR and NOT.

Yes, I agree too – definitely a good change.

Sorry to have been so long getting back to this! It was relegated to my back burner for a while, but I've now found time to address the review comments so far.

But otherwise this is a very easy-to-check change compared to the previous ones on the same lines :-) LGTM.

Nit: your summary line and commit message don't match :-) One of them says "changed from target to preprocessor" and the other vice versa.

Ah, yes, now I see it. Thanks for clarifying.

Hmm, yes, sorry about that. Using -print-after-all, it looks as if the useful parts of the loop body are vanishing during InstCombine, and indeed the ConstraintElim pass isn't running at all.

Hello. I think this recent change is the one that's caused clang to begin optimizing C code of the following type into the empty function:

extern const size_t start[], end[];
for (const size_t *p = start; p != end; ++p) { /* do stuff */ }

No, I think this was exactly the mistake on my part that it looks like. Good catch, and thanks for fixing it!

Going by the new test llvm/test/CodeGen/Thumb2/pipeliner-inlineasm, the two pieces of code don't conflict if both are present. But the code in the pipeliner also doesn't seem to be necessary any more. So I've revised this patch to remove it, and do all tie propagation centrally in MachineInstr's copy constructor.

In D135434#3845431, @dmgreen wrote:

This sounds OK to me, and is probably how I would fix this. But it will effect more than just the MachinePipeliner, anything that goes through CloneMachineInstr. Looking at where that is uses, that seems like most the uses should be OK. I'm don't think many backends will use these implicit tied uses, and copying them sounds more correct during a clone.

Absolutely, I agree. Off the top of my head, the most obvious continuing use case for x86-16 is first-stage bootloaders that the PC BIOS runs in real mode. I've no idea what tools people typically use for those these days, but I wouldn't have a hard time at all believing that it might turn out to be a hodgepodge of bits and pieces from all over the place.

Supplementary discussion:

Adjusted check prefixes, and translated all yaml2obj inputs into llvm-mc inputs (which in all cases fails with the old llvm-objdump, i.e. still produces an object file that successfully tests the changed behaviour).

(Ah, that's where the one last unticked Done box was hiding.)

I think I've now addressed all your review comments, including adding a demonstration of alphabetical order vs demangling.

Moved the check for data symbols to before we choose symbols to display, so that the same check can control which symbol is printed and how the data after it is disassembled.

In D131589#3728128, @jhenderson wrote:

I think one bit of testing you still need is showing whether the demangled or raw name is used for the priority ordering of symbol names.

I've left most of your comments un-replied-to so far, because I need to think harder about the choice of symbols to display, as mentioned in one of my inline comments below.

Added another test to check that the new data vs code priorities work.

Updated handling of STT_OBJECT symbols as discussed. Also added a comment about the confusing double loop setting up the demangled symbol names, since the next person might also wonder why it's not being done in one step.

Ping again. If I'm reading the libc++ group members page right, this patch needs to be looked at by at least one of: @EricWF @erik.pilkington @ldionne @mclow.lists @phosek @compnerd @urnathan . Is there somewhere else I should be asking, to bring it to the attention of the right people?

Addressed all review comments (I think) other than the question of STT_OBJECT priority versus other kinds of symbol.

In D131589#3714216, @rafauler wrote:

I'm not the code owner of these disassembly tools

The clang code change looks reasonable to me, but I'm not the most expert in that area.

Oh, oops, caught out again :-( I really must stop assuming that a Phab comment saying "LGTM" actually marks the patch as accepted. Sorry about that. Will revert if you need me to.

I didn't know about that previous patch at all, but off the top of my head, yes, I don't think it should be necessary at least for ELF files, because the combination of ELF header endianness and EF_ARM_BE8 flag is enough to autodetect the right code endianness in all cases.

Ping. Can someone in the libc++abi blocking reviewer group take a look at this patch, please?

Oh yes, sorry about that. Switching a little too fast between projects with completely different code review practices :-(

I love it! I'm generally frustrated by any software that says "what even is this, I've never heard of it" when it could so easily change it to "I have heard of this but it's not allowed in this particular context because reason". If nothing else, it means you get different error messages for "misspelled an intrinsic" and "used one from SVE version n+1".

Ah, yes, I misread what the test changes were about – I didn't notice that you'd only just introduced the scalar error context, and thought you were enabling all those operations in at least one situation, rather than changing the form of the check that they were unconditionally disabled.

What actually happens if you try this? Normally I'd expect that if a thing is forbidden in the front end it's because otherwise it would lead to a codegen crash or something similar. But this patch is only removing the restriction, and not adding any support. Is that part already done, and bf16 operations will get automatically converted into some libcall that already exists, in the absence of any better approach?

I wrote this patch in order to use it myself. Naturally the problem I was trying to debug turned out to be embarrassingly trivial – I hadn't made a subtle mistake in the macro syntax, I'd put my -D options on completely the wrong command line. But now I've written the patch anyway it seems a pity to waste it :-)

Addressed all review comments, I think.

Updated with arc lint style fixes.

Corrected that comment.