User Details
- User Since
- Jul 12 2017, 1:23 AM (298 w, 1 d)
Fri, Mar 24
Mon, Mar 13
Fri, Mar 10
Thu, Mar 9
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'!
Feb 21 2023
Feb 20 2023
Feb 16 2023
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.
Feb 15 2023
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!
Feb 9 2023
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.
Feb 8 2023
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.
Feb 6 2023
LGTM, with a small commenting nit.
Feb 1 2023
Jan 27 2023
Added draft release notes, and an extra comment in the modified test file.
Jan 20 2023
Jan 17 2023
"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.
Jan 12 2023
Dec 6 2022
Dec 5 2022
Yes, I agree too – definitely a good change.
Nov 29 2022
Nov 28 2022
Nov 17 2022
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.
Nov 15 2022
Nov 3 2022
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.
Oct 24 2022
Ah, yes, now I see it. Thanks for clarifying.
Oct 18 2022
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 */ }
Oct 13 2022
Oct 12 2022
No, I think this was exactly the mistake on my part that it looks like. Good catch, and thanks for fixing it!
Oct 11 2022
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.
Oct 10 2022
Oct 7 2022
Aug 24 2022
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:
Aug 22 2022
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).
Aug 19 2022
(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.
Aug 17 2022
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.
Aug 16 2022
Added another test to check that the new data vs code priorities work.
Aug 15 2022
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.
Aug 12 2022
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?
Aug 11 2022
Addressed all review comments (I think) other than the question of STT_OBJECT priority versus other kinds of symbol.
Aug 10 2022
The clang code change looks reasonable to me, but I'm not the most expert in that area.
Aug 8 2022
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.
Aug 5 2022
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?
Aug 4 2022
Oh yes, sorry about that. Switching a little too fast between projects with completely different code review practices :-(
Aug 3 2022
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.
Aug 2 2022
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.
Aug 1 2022
Updated with arc lint style fixes.
Jul 29 2022
Corrected that comment.