LGTM Cheers

I believe we also write tests for timeout bugs, for instance https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/InstCombine/phi-timeout.ll. Could you also add one?

I think this patch generally LGTM. But I would love to know if plugin support or third-party extension in general is part of your roadmap, if that's the case...

FIXME: This may be move to llvm/include/llvm/TableGen/Main.h

In D144351#4138905, @chapuni wrote:

• Move decl of EmitDecoder()to TableGenBackends.h

In D144189#4134864, @qiongsiwu1 wrote:

In D144189#4132940, @myhsu wrote:

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

just wondering if there is any reason you chose not to implement this as a subtarget feature?

LGTM with minor suggestions

LGTM thanks!

I believe some tests are missing like those related to bytecode. Also could you attach diff with more context as instructed here.

My Phab handle is different from my Discourse account

just skimmed through the patch but I think either ISelLowering (in which case you probably don't need the custom ISD) or the pseudo instruction expanding Pass will be a better place to put the main logics, rather than AsmPrinter.

LGTM on 68k part

In D142821#4091312, @boschmitt wrote:

Yes, I need help pushing the patch as I don't have commit access.

Let us know if you need helps pushing the patching

LGTM thanks :-)

Could you add a test (not a multi-line one, just a pipeline string with white spaces) in test/Pass/pipeline-parsing.mlir as well? It should be easy

In D142364#4079397, @DavidSpickett wrote:

This path definitely sounds easier to me :-) And yes, I'm the sole author and copyright holder of SQLGen. How can I give such permission to you? Send an email?

Just needs to be some record of it, so your comments here count for that.

LGTM, thank you for creating this tutorial! As I also mentioned in our previous email communication, I think this really helps the broader community including beginners to learn TableGen programming.

In D142364#4073657, @DavidSpickett wrote:

I should note that this does not require the tablegen kernel, it's the default iPython kernel. You will need to tell it how to find llvm-tblgen though.

@myhsu This is what we talked about over email last year.

There is a licensing issue I need you to clarify if you are ok with. Your C++ work is Apache 2.0 and this would be a derivative work of that, licensed the same. Which is a bit awkward to do here, perhaps it would be enough to include a copy of the license at the start of the tutorial, I'd have to check with an expert.

• Pull getCustomCoders out of anonymous namespace
• Report a fatal error if a encoder or decoder directive is not followed by a function name.

In D142079#4071791, @RKSimon wrote:

In D142079#4071770, @Paul-C-Anagnostopoulos wrote:

The TableGen Programmer's Reference does not include descriptions of backend facilities.

So maybe BackGuide.rst ?

Addressed feedbacks

Addressed feedbacks

You can use llvm::SetVector if elements need to be unique

With D142079 and D142080 we can finally decode immediates of all sizes in the correct way.

Congratulation!

In D140695#4018127, @0x59616e wrote:

FYI the pre-merge checks seem to have some issues. Here is the output:

Command Output (stderr):
--
/var/lib/buildkite-agent/builds/llvm-project/clang/test/Preprocessor/predefined-arch-macros.c:4322:28: error: CHECK_M68K_GCC_ATOMICS: expected string not found in input
// CHECK_M68K_GCC_ATOMICS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
                           ^
<stdin>:1:1: note: scanning from here
#define _ILP32 1
^
<stdin>:62:1: note: possible intended match here
#define __GCC_ATOMIC_CHAR_LOCK_FREE 1
^
 
Input file: <stdin>
Check file: /var/lib/buildkite-agent/builds/llvm-project/clang/test/Preprocessor/predefined-arch-macros.c
 
-dump-input=help explains the following input dump.
 
Input was:
<<<<<<
              1: #define _ILP32 1
check:4322'0     X~~~~~~~~~~~~~~~~ error: no match found
              2: #define __ATOMIC_ACQUIRE 2
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
              3: #define __ATOMIC_ACQ_REL 4
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
              4: #define __ATOMIC_CONSUME 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
              5: #define __ATOMIC_RELAXED 0
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
              6: #define __ATOMIC_RELEASE 3
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
              .
              .
              .
             57: #define __FLT_MIN__ 1.17549435e-38F
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             58: #define __FLT_RADIX__ 2
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~
             59: #define __GCC_ATOMIC_BOOL_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             60: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             61: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             62: #define __GCC_ATOMIC_CHAR_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:4322'1     ?                                      possible intended match
             63: #define __GCC_ATOMIC_INT_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             64: #define __GCC_ATOMIC_LLONG_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             65: #define __GCC_ATOMIC_LONG_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             66: #define __GCC_ATOMIC_POINTER_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             67: #define __GCC_ATOMIC_SHORT_LOCK_FREE 1
check:4322'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              .
              .
              .
>>>>>>
 
--

LGTM Thanks!

I just quickly tried this with /W3 on MSVC 2019 and the warning no longer appears. Cheers

I think you'd addressed all errors from the buildbot failure.

With this patch we need a custom decoder for 32-bit immediate value or disassembler won't be able to recognize the correct word ordering. I will add custom decoder support for variable length instruction in separate patches.

LGTM thanks

LGTM thank you! Please also include cmpxchg and rmw in the title.

LGTM thanks

I only have minor comments. Otherwise LGTM. Cheers :-)

I read your patch descriptions with great interest and kudos on listing all cases. I generally agree with your instrument comment / analysis region. I think it's the most practical solution we have for now and also opens a door for creating other kinds of MCA extensions in the future.

I think the patch is in good shape now. I only have some minor comments. Thanks for the works :-)

In D136525#3901614, @0x59616e wrote:

Thanks for all of your edifying comments. The path is getting clearer. Here is my understanding. Correct me if I'm wrong.

1. __atomic_* is allowed to use lock, whilst __sync_* is lock-free.
2. Non-lock-free function is NOT allowed to use simultaneously with the lock-free instruction or function, otherwise the lock may not be respected (i.e. race condition may occur)
3. atomic_compare_and_swap needs to be transformed to __atomic_* if we don't have CAS support.

Let me try to answer most of the questions at once. But first, here is the workflow I would do:

1. For 68020 and later, setMaxAtomicSizeInBitsSupported(32) and setMaxAtomicSizeInBitsSupported(0) otherwise. AtomicExpandPass then replaces everything not in the size ranges with __atomic_*.
2. AtomicLoad, AtomicStore and AtomicCmpXchg in target >= 68020 will be lowered to native instructions.
3. Mark every other ISD::ATOMIC_* as LibCall, this effectively lowers them into library calls to __sync_*.

I think it makes sense to assume m68k always runs on uniprocessors. GCC / libgcc and m68k port of Linux also makes the same assumption. For instance, gcc simply lowers atomic fence into asm volatile("" ::: "memory") (meaning the only thing we need to do is preventing compiler from reordering across the fence).

In D136808#3890589, @Rot127 wrote:

So as long as one backend is updated at a time there is no problem. Since backends do not share printing logic in any way.

Yes I was simply wondering if it's possible to update a single backend a time. Good to know now.

I only skimmed through the patch but I found this pretty interesting. To my best understanding, you're trying to add another layer of abstraction between disassembler generation logics and the code printer. Such that downstream projects like Capstone only need to maintain a single printer code (e.g. PrinterCapston.cpp) rather than hundreds of lines of code spanning across DecoderEmitter.cpp. If this is the case, I strongly encourage you to upstream most of the stuff here except PrinterCapstone.cpp.

Just some drive-by comments

Agree with @nikic . I think it will be better for this patch to have support for CmpXchg using CAS (for 68020 and later) and fallback to library solution for older CPUs.

Do not broadcast when the operand types are different

LGTM, thanks

Thank you for fixing this bug!
As I mentioned in #57660, I feel like we should fix the assertion instead

typo in the title: std::bittest -> std::bitset

LGTM on 68k side.
Thanks for fixing.

I don't think this is Clang-specific, could you update the title?

LGTM
Thank you! I only have some minor comments.