The 128-bit logic doesn't look quite right to me. I think what should be used there is:

• "r" constraint - GPR register pair
• "f" constraint - FPR register pair if hard-float, error if soft-float
• "v" constraint - single VR if vector support enabled, error otherwise

LGTM, thanks for taking care of s390x!

I believe the imm32zx6 operand definition is now unused and should be removed as well. Otherwise this LGTM.

LGTM as well, thanks!

I believe the situation between the __sync and __atomic builtins is a bit different, though. For __atomic, if the compiler cannot prove correct alignment, it will emit a library call. This should always do the correct thing, however may have a performance overhead - which is what the warning says.

Fix by checking this attribute on the current function. Alternatively, one could try the callee first, but this is pointless, since one should not be mixing hardfloat and softfloat code anyway.

LGTM

There are probably other SystemZISD nodes that could go in here, but I am not quite sure how this is supposed to work: It seems to me that some kind of undef/poison value is recognized and at some point a freeze instruction is inserted in the IR. Then the DAGCombiner optimizes it away because there is a target node that now says that it is never undef..? This is contradictory but probably still worthwhile somehow, but I can't quite figure out how..?

These points all look good to me, but shouldn't the __int128 alignment change (https://reviews.llvm.org/D130900) also be mentioned?

Thanks! Minor commit nit inline, otherwise this now LGTM.

In D141409#4085519, @jonpa wrote:

There is no difference with GCC between z10/z13 here, so this may not be actually necessary as far as I can see.

(Patch now matches GCC behavior also in the single element struct case.)

In D141409#4083422, @jonpa wrote:

The old behavior is not explicitly specified by any ABI doc, so we can really only go after what GCC actually used to do. And in fact GCC used to (and still does!) simply place such arguments on the stack and passes a hidden pointer to the stack slot - without any dynamic stack realignment, so it in fact only guarantees 8 byte alignment. So I think it is *safe* to assume that using 8 bytes always is OK - and in that sense it is safe to omit the GNU attribute.

OK - I have been assuming that *all* vectors in memory should be aligned the same, but if that's not necessarily true for wide vector arguments that are forced to the stack, I guess that explains this. Should I update the patch to do just like GCC in these cases (and not emit the attribute)?

Well, with this test case:

typedef __attribute__((vector_size(32))) int v8i32;

In D141409#4083017, @jonpa wrote:

Review addressed - hopefully this time with a better result.

I think the patch now does what I want it to, while I found some test cases while checking against GCC which I am a little unsure of:

(using GCC 12.2.1, 20220901)

In D141187#4054105, @jonpa wrote:

LegalizeTypes expands the i128 result of e.g. an AtomicLoadAdd node, which is done by DAGTypeLegalizer::ExpandAtomic(), which results in a library call. There are no tests for the atomic operations with i128, but it should work due to the TypeLegalizer. i128 is not a legal type so I guess therefore they are all expanded this way.

I could imagine that it might be beneficial to use the AtomicExpandPass to use the target instructions whenever possible, and libcalls otherwise. However, I am unsure about this: The AtomicExpandPass seems to do this (emit the libcalls only if needed, after checking alignment as well), but I see this in the Atomics document:

"One very important property of the atomic operations is that if your backend supports any inline lock-free atomic operations of a given size, you should support ALL operations of that size in a lock-free manner...it is invalid to implement atomic load using the native instruction, but cmpxchg using a library call to a function that uses a mutex, atomic load must also expand to a library call on such architectures, so that it can remain atomic with regards to a simultaneous cmpxchg, by using the same mutex."

I guess I must be missing something somehow...

Huh. We still get the lr :-( That now seems an inefficiency in the RXSBG optimization pass, so it's not a problem with this patch, which now LGTM. Thanks again!

I was actually thinking of the case

struct arg
  {
     char x;
     v8i32 y;
  };

In D140988#4044864, @tuliom wrote:

In D140988#4042746, @uweigand wrote:

CWD1 = efpc() & 3
CWD2 = (CWD1 ^ (CWD1 >> 1)) ^ 1

@uweigand This is not generating the right output.

With the changes pointed out by @nikic this looks good to me.

I don't think this is quite correct, as we do support 128-bit atomics on properly aligned addresses. In any case, the back-end implements atomic load, store, and cmpxchg for i128. We do not currently implement atomicrmw, but that could be handled in terms of cmpxchg of course. For non-aligned addresses, we do need to fall back to the library routine, however.

Thanks for working on this!

LGTM, thanks!

In D133949#3987273, @jonpa wrote:

Don't know much about the signedness of the different args, which of course is important to get correct. I guess I have used zero-extension mostly - it would be good to get this reviewed properly. In OMPKinds.def I added ZExt to all narrow int args, except SExt for args marked with /* Int */.

LGTM, thanks!

In D139444#3983679, @uweigand wrote:

In D139444#3982205, @probinson wrote:

If you can tell me the platform.system() value to look for to detect z/OS, I can do that.

I don't know - this value is defined by the Python implementation on z/OS. On most platforms, this matches the value returned by "uname -s", but I don't know what this is on z/OS either.

In D139444#3982205, @probinson wrote:

If you can tell me the platform.system() value to look for to detect z/OS, I can do that.

In D139444#3975182, @probinson wrote:

The changes in this patch assume that there aren't any possible suffixes after the -zos part of the triple (no version numbers, like you might find with darwin or macos, and nothing like -elf or -eabi like some targets have). If there are suffixes, I'll happily revise to put {{.*}} after everything.

See one inline comment, otherwise LGTM now. Thanks!

LGTM, thanks!

Looking into this a bit more, the argument to GET_CCMASK is known to be a boolean value (either 0 or 1), so we don't need to verify that after all, and can just accept truncates in general.

LGTM, thanks!

In D137044#3912908, @efriedma wrote:

Move this check into isPC32DBLSymbol, maybe?

In D137044#3909116, @efriedma wrote:

I'm very confused how you expect this to work... messing with the linkage of global variables is almost certainly going to explode. If you need emit special sequences to access global variables with internal linkage, I'd expect the backend to handle that (along the lines of how -frwpi works on ARM).

In D137044#3906898, @MaskRay wrote:

In D137044#3901481, @uweigand wrote:

In D137044#3901025, @MaskRay wrote:

Can you describe the motivation adding this option? Is it for a FDPIC ABI? Removing the assumption that text and data has a constant offset generally make the code worse for data access. This is almost assuredly a bad idea if the system has a memory management unit.

This is intended to be used for "kpatch" - the Linux kernel live-patching facility. This allows replacing parts of the kernel code with an updated version at runtime. However, the *data* segment cannot be moved since it is already in use (and pointers to the data may be around), and so the replacement text segment will have to refer to the original data segment. Since the replacement text segment will generally end up at different addresses than the original one, this means the compiler cannot make assumptions about the relative distance from code to data.

Of course this in general results in worse code, but that is considered an acceptable cost to enable the live-patching feature. Note that GCC already supports the same option with the same semantics, used for the same purpose. The intent of supporting it in clang as well is simply to optionally enabling building the Linux kernel with clang instead of GCC while preserving the full set of kernel features.

I know the existence of livepatch in the Linux kernel though unfamiliar with it. The kernel doesn't use -mno-pic-data-is-text-relative right now for any port. Why does SystemZ need -mno-pic-data-is-text-relative for livepatch?

In D137044#3901025, @MaskRay wrote:

Can you describe the motivation adding this option? Is it for a FDPIC ABI? Removing the assumption that text and data has a constant offset generally make the code worse for data access. This is almost assuredly a bad idea if the system has a memory management unit.

LGTM, thanks!

LGTM now! Thanks for making those changes.

This fixed the s390x build bot again. Thanks!

This is breaking the mlir-s390x-linux builder (https://lab.llvm.org/buildbot/#/builders/199/builds/11463):

CMake Error at /home/uweigand/sandbox/buildbot/mlir-s390x-linux/llvm-project/mlir/lib/ExecutionEngine/SparseTensor/CMakeLists.txt:12 (add_library):
  add_library INTERFACE library requires no source arguments.

In D136040#3862386, @pengfei wrote:

Sure, it is possible. But at least for now, there's no real target requires it. Checked with grep -rwn 'llvm.prefetch.*i32 0\s*)' llvm/test/CodeGen/.

In D136040#3862225, @pengfei wrote:

3. Add semacheck for prefetch write to instruction cache;

I think the affected ARM and SystemZ tests are not valid before. Could @t.p.northover and @uweigand help to have a look?

The SystemZ change is a clear improvement, so that part LGTM. Thanks!

LGTM, thanks!

In D91620#3836854, @ro wrote:

This seems to cause another regression, apparently only visible in cross builds: clang-s390x-linux buildbot: a link failure like this:

SystemZ content LGTM. Thanks!

Sorry for the delay, I was traveling last week. Thanks for the use case, it's good to have some validation that the output in this form is actually usable.

This LGTM now, but maybe some of the PowerPC reviewers would like to comment as well. @nemanjai ?

LGTM, thanks!

I think it is correct to implement this in Clang. Note that on SystemZ (another big-endian platform), we also implement this in EmitVAArg. Of course the details are different since we're not using emitVoidPtrVAArg on that platform.

This causes builder failures on s390x to reappear:
https://lab.llvm.org/buildbot/#/builders/199/builds/9247

In D129868#3751791, @antoniofrighetto wrote:

@uweigand, mostly third-party clients such as disassemblers and pretty printers, as per official documentation. Already supported by ARM as well as x64.

Can you explain what this is, and where this marked-up assembler could be useful? I see so far only ARM supports that feature, but I don't really understand the point ...

In D131747#3740387, @rriddle wrote:

@uweigand Thanks for the ping, it's possible big-endian is fine up to the point at which some of the tests fail (I haven't had time to setup a venv to test everything out). UNSUPPORTED is likely a better check than XFAIL there (I just copied from our other s390x broken test)

Hi @rriddle , as of commit https://github.com/llvm/llvm-project/commit/93cf0e8a28e8c682f65d3e5c394d1eb169ca09ce the s390x build bot is still red due to "unexpected success":

XPASS: MLIR::invalid-string_section.mlir
XPASS: MLIR::invalid_attr_type_offset_section.mlir
XPASS: MLIR::invalid_attr_type_section.mlir
XPASS: MLIR::invalid-structure.mlir
XPASS: MLIR::invalid-ir_section.mlir
XPASS: MLIR::invalid-dialect_section.mlir

(see https://lab.llvm.org/buildbot/#/builders/199/builds/8674)

It seems that SPEC builds identically both for z15 and zEC12, which is promising.

LGTM, thanks!

In D130900#3693772, @jonpa wrote:

@uweigand I also added tests for the alignment of a vector depending on the presence of the vector facility. I am not sure if I have run into something weird here, but I left the test and output this way for now so you can see what I mean: If I use -mattr=-vector the alignment becomes 16 for the vector, but not if this is specified in the function attributes only (see @f2).

In D77804#3681806, @RKSimon wrote:

I think I've covered all the remaining regressions now - D129765 has cleaned up a number of annoying cases - including the SystemZ v3i31 copy test!

In D129856#3657718, @smeenai wrote:

process_vm_readv was introduced in Linux 3.2; is it okay to use it unconditionally for you? (aarch64 support was only added in Linux 3.7, so we were guaranteed to have the syscall available for it.)

IMO the fun2 regression probably shouldn't block the patch from being merged. I've looked into the sequences, and actually neither of them is even close to optimal.

LGTM, thanks!

The s390x code path should likely follow suit, but I don't have the
hardware to be able to test that, so I didn't modify it here either.

Given Anton's comment, and since nobody else objected, I'm going to approve this. It is indeed a nice improvement for our backend. LGTM.

LGTM, thanks!

LGTM, thanks!

SystemZ mlir builder is now back to green. Of course, the underlying issue still needs to be fixed to we can remove the XFAIL again.

I've now committed both changes in separate commits. Thanks!

We have to fix the alignment tag for real, otherwise there will be failing assertions all over the place. With that fix in, there's still a failing test case - that one could indeed be XFAILed for now.

This commit has broken the mlir builder on s390x-ibm-linux: https://lab.llvm.org/buildbot/#/builders/199/builds/6417

LGTM, thanks!

LGTM

In D125676#3563470, @jpienaar wrote:

Does this also show with an i4 test case? (i1 is special in a few ways, so having other sub CHAR_BIT test case would be good)

Add test case.

FYI @nemanjai

From a SystemZ perspective this looks good to me (and a clear readability enhancement). I'd still like to hear comments from other backend maintainers whether this common tablegen feature might be useful to them as well ...

From a SystemZ perspective this looks good to me.