address feedbacks

I was confused with the absence of some of my fix in the test after rebasing to master. It turns out that this has been done by Min.

update diff

remove comments

Sorry for holding this for so long. I will come back next week since we have a a 5-day holiday in Taiwan.

Add some comments

A few minor adjustments are required.

Shoot. I mixed it up with my own one. I'm sorry.

A few minor adjustments are required.

LGTM. Thanks

The access model type is disambiguated at ISelLowering, but it still needs to be expanded in AsmPrinter. I am uncertain how to express the Global Offset Table in MachineInstr.

Add disassembler test

• Add disassembler test
• Move assembler test into MxBiArOp_RFRM.s

Plan to update the code per the advice received

In D143317#4104732, @glaubitz wrote:

Doesn't seem to build at the moment:

LGTM. Thanks for this amazing work ;)

This amazing work LGTM. Thanks !

The CI seems OK. My first LGTM is given to you.

Also, could you add "Fixes #58974" in the commit message so that the item will be closed automatically upon commiting ? Thanks

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
^

I like your idea, but I'm not a big fan of the code structure. It seems that we have to add a plethora of virtual functions for every backend in a single PrinterInterface class ? Wouldn't this break the 'open-close principle' and 'Interface segregation principle' ?

Thanks a lot for all of your benign help ;)

Thanks a lot ;)

update diff:

• Expand atomic-rmw to atomic-compare-and-swap on target >= M68020
• address feedbacks

Replace IsM680x0 with AtLeastM680x0

update diff:

• We transform all atomic instruction to __atomic_* for sizes > 32
• Otherwise, lower to either native instruction or __sync_* function call.

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

one more question :

In D136525#3899548, @efriedma wrote:

The atomic width is never a property of a specific instruction. Either *all* atomic ops of a given width are lock-free, or *all* atomic ops of a given width need to be transformed into __atomic_* libcalls.

address feedbacks

address feedbacks

Add pipeline.ll

Add support for atomicrmw and cmpxchg

I dig into the libatomic.a, here is part of the result:

00000000 <__atomic_store_4>:
   0:   206f 0004       moveal %sp@(4),%a0
   4:   20af 0008       movel %sp@(8),%a0@
   8:   4e75            rts

Thanks for this patch !

In D136525#3880337, @efriedma wrote:

Are multi-processor m68k computers a thing? I can't find any reference to such a thing existing, but the manual indicates that the processor was designed to allow it. If it does exist, m68k probably needs to use sequences similar to x86. (x86 didn't have any barrier instruction for a long time, but a "lock" instruction has the right semantics.)

Take my word with a pinch of salt

Embark on the CAS/RMW instruction.

In D136525#3877522, @nikic wrote:

Preferably this should also include the implementation for atomic RMW/CAS instructions to prove that this lowering is legal. If native or at least kernel-supported CAS is not available, then atomic load/store needs to use libatomic (possibly subtarget dependent).

It would also be good to include relevant quotes from the ISA manual -- atomicity of load/store is usually a given, but do they also guarantee a seq_cst ordering without a memory barrier?

(Disclaimer: I'm not familiar with m68k, just covering the usual atomic lowering legality questions.)

I can only nitpick some of the peripheral issues since I have no knowledge in most of the part of clang. Perhaps implementing the new standard feature is too arduous for a tyro like me. It's great to see the real expert to complete this.

Thanks for all of your kindly help ;)

In D133850#3849932, @mingmingl wrote:

Hi, I realize D133280 is a diffbase from the 'stack' UI, and actually I never figured out how to send stack reviews over others' patches, so didn't know if a) or b) is a simpler procedure
a) for me to submit D133280 first b) you could just commit all changes reviewed in this patch.

Since this patch supersedes the test-only patch, I'm fine with a) or b), whichever makes the procedure simpler. Just let me know :)

Address friendly feedbacks

Address friendly feedback and update all the affected tests

Address kindly feedbacks. No major change in the core algorithm.

There's no update on my side.

In D133850#3815818, @mingmingl wrote:

In D133850#3814014, @0x59616e wrote:

bitcast is handled in this diff.

To handle bitcast, we need this observation: uzp1 is just a xtn that operates on two registers simultaneously.

For example, given the following register with type v2i64:

LSB______MSB

x0 x1	x2 x3

Applying xtn on it we get:

x0	x2

This is equivalent to bitcast it to v4i32, and then applying uzp1 on it:

x0	x1	x2	x3

=== uzp1 ===>

x0	x2	<value from other register>

We can transform xtn to uzp1 by this observation, and vice versa.

This observation only works on little endian target. Big endian target has a problem: the uzp1 cannot be replaced by xtn since there is a discrepancy in the behavior of uzp1 between the little endian and big endian. To illustrate, take the following for example:

LSB________MSB

x0	x1	x2	x3

On little endian, uzp1 grabs x0 and x2, which is right; on big endian, it grabs x3 and x1, which doesn't match what I saw on the document. But, since I'm new to AArch64, take my word with a pinch of salt. This bevavior is observed on gdb, maybe there's issue in the order of the value printed by it ?

Whatever the reason is, the execution result given by qemu just doesn't match. So I disable this on big endian target temporarily until we find the crux.

Take this with a grain of salt

My understanding is that, 'BITCAST' on little-endian works in this context since the element order and byte order is consistent that 'bitcast' won't change the relative order of bytes before and after the cast.

Use LLVM IR <2 x i64> as an example, we refer to element 0 as A0 and element 1 as A1, refer to the higher half (MSB) as A0H, and lower half as A0L

For little-endian,

1. A0 is in lane 0 of the register and A1 is in lane1 of the register, with memory representation as

0x0 0x4  0x8  0xc
A0L A0H A1L A1H

2. After bitcast <2 x i64> to <4 x i32> (which is a store followed by a load), the q0 register is still A0L A0H A1L A1H and LLVM IR <4 x i32> element 0 is A0L

For big-endian, the memory layout of <2 x i64> is

0x0 0x4 0x8 0xc
A0H A0L A1H A1L

So after a bitcast to <4 x i32>, q0 register becomes A0H A0L A1H A1L -> for LLVM IR <4 x i32>, element 0 is A0H -> this changes the shuffle result.

p.s. I use small functions like https://godbolt.org/z/63h9xja5e and https://gcc.godbolt.org/z/EsE3eWW71 to wrap my head around the mapping among {LLVM IR, register lanes, memory layout}.

Address kindly feedback.

bitcast is handled in this diff.

In D133850#3809437, @efriedma wrote:

I have no idea regarding how gdb decides to print it... but for the general issue, hopefully https://llvm.org/docs/BigEndianNEON.html helps?

I have a question : how does the SIMD instruction view the vector register in big endian mode ?