Finally getting back to this patch - sorry for the long delay.

I don't understand the changes to the SystemZ test cases. The new behavior seems to be simply wrong?

In D114194#3141215, @jonpa wrote:

Not sure what to do with the testing: should we do something similar as a 'Large' folder as is already done for the branch relaxation tests?

SystemZ changes LGTM.

In D113894#3131822, @thakis wrote:

In D113894#3131809, @uweigand wrote:

Thanks @hans for the heads-up, I do indeed think this patch would break inline asm on z/OS.

Do you have any test coverage of z/OS? All tests pass with this as far as I can tell.

In D113341#3131566, @jonpa wrote:

I tried this and it was simple enough - with just an increment in a single place in getDispOpValue(). However, encodeInstruction() is an overriden const function, so clearing MemOpsEmitted wass not permitted. Then it occured to me that if we really can make this broad assumption, we could just check the size of the Fixups vector and that way differentiate between the first and second fixup.

Thanks @hans for the heads-up, I do indeed think this patch would break inline asm on z/OS.

LGTM, just a few minor nits inline. We also should do a performance validation run before committing this.

In D113341#3126096, @jonpa wrote:

• Mapping of MI:OperandIndex to fixup byte offset: I added a TSFlags bit 'MemMem' which is checked for in getDispOpValue(). From this flag it is known that there are two memory operands, with displacements starting at bit 20 and 36. For the matter of choosing which one of these are in question in getDispOpValue(), based on the OpNum, I was not sure if this could be hard coded as well in the formats file, or if it would be simpler to check this at compile time the way that is currently done..?

SystemZ changes LGTM - I guess we're always in the "SingleInst" case.

I think it would be a bit simpler and clearer to handle the Src vs. Dest address special case for memset in a single place at the beginning. Something along the lines of:

This looks like a nice improvement. LGTM.

LGTM, thanks!

LGTM, thanks!

LGTM, thanks!

LGTM, thanks!

LGTM, thanks!

The SystemZ back-end changes LGTM.

LGTM as well.

LGTM

LGTM, thanks!

LGTM, thanks!

In D109362#3013253, @anirudhp wrote:

• Reduced the number of test lines in target-data.c, since we don't have to check for every combination of arch,cpu for the SystemZ target (for both elf and z/OS)

LGTM now, thanks!

In D91944#3009868, @cchen wrote:

The SystemZ issue is due to the fact that we assumed that device(cpu) should be evaluated to true and device(gpu) should be evaluated to false in the test so the test should be fixed by specifying the triple. (https://github.com/llvm/llvm-project/commit/3679d2001c87f37101e7f20c646b21e97d8a0867)

Two minor nits inline, otherwise I agree we should go with that approach now.

Looks like this was committed again, breaking the SystemZ build bots once again:
https://lab.llvm.org/buildbot/#/builders/94/builds/5661

In D109657#3008172, @jonpa wrote:

I believe %reg should be treated equivalently to 0(%reg) for any type of address, including those with length or vector index, so this should be fine. However, it would be good to validate that behavior against GAS, and also add a few test cases for instructions with those address types.

For addresses with a Length field, it seems GAS does require the full address with the explicit displacement as well:

        xc      0(32,%r2), 0(%r2)
        xc      0(32,%r2), %r2
#       xc      (32,%r2), %r2    # not accepted by GAS
#       xc      32,%r2, %r2      # not accepted by GAS

This LGTM now.

In D109362#3000284, @anirudhp wrote:

In D109362#2999688, @uweigand wrote:

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

At a future point, we will be changing the local and global prefixes. At that point we would still need a separate MM_GOFF field. I feel it would be a bit better to enforce it from now itself.

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

In D109660#2997903, @jonpa wrote:

SystemZTargetStreamer added with derived and registered SystemZTargetAsmStreamer and SystemZTargetELFStreamer classes. The object TargetStreamer does not output anything in emitMachine(), which I don't think it should, or?

LGTM.

Does such a directive need to be output again from the parser in case output is back into textual form (filetype=asm)? This is currently not done by the patch - I see that these are dropped after running GAS | objdump... (object format). I guess in that case either MCTargetStreamer is needed, or possibly some RawText emission..?

This patch is not checking the expected kind of address (MemKind) or HasLength / HasVectorIndex - would that be better?

Ah, I see, we've have to implement a MCTargetStreamer first ... In any case, I don't have a strong opinion on this, if the common code change is acceptable, that's of course fine with me. A back-end only solution using a MCTargetStreamer would also be OK.

In D109513#2994352, @jonpa wrote:

In D109513#2994113, @uweigand wrote:

In D109513#2994077, @jonpa wrote:

In D109513#2992888, @aeubanks wrote:

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Hmm ... that makes me wonder: can we instead just use the existing emitConstantPools in the SystemZ back end to output the EXRL instructions? They are in effect a form of constants ... That shouldn't require any common code changes then.

I read in the original bug report: "...adding the XC instructions in some other text section out of the eyes of debug is not a good option for z/OS (details excluded)". Therefore I thought they better be placed in the text section...

In D109513#2994077, @jonpa wrote:

In D109513#2992888, @aeubanks wrote:

do you think it'd make sense to merge emitConstantPools into emitSectionEndings?

How about moving emitConstantPools() into the default implementation of emitSectionEndings() like this..?

Formatting looks good to me now as well. Still LGTM.

SystemZ specific parts LGTM, but it would be good to have someone else review the common code / IR changes as well.

This LGTM, but it would be good to see some full benchmark results as well.

As you discuss in the bugzilla, this seems a good compromise between using the (larger) RC associated with the type when available, and falling back to the (minimal) RC associated with the physreg if we have a "weird" type. LGTM.

LGTM

LGTM

I tried now to do it in LowerCall(), but as I did not find any way to set a flag at Module scope

This looks good to me, but we should verify the performance using a full run.

In D106782#2905801, @MaskRay wrote:

In D106782#2905730, @uweigand wrote:

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

A latent problem triggered by the change. Fixed by c5d8bd5a35cbd325c6ccd42afa91bad06d261f07

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

The SystemZ specific changes all look good to me now, thanks!

See inline comments ... otherwise the SystemZ platform-specific parts look good to me.

LGTM, thanks!

LGTM, thanks!

In D105067#2857971, @jonpa wrote:

I don't think this implementation of vararg support is quite complete. On the caller side, you check for variable arguments to any direct function with a prototype. However, this misses the case where an *indirect* function call (using a function pointer) has variable arguments.

This also completely misses the *callee* side: every function that takes variable arguments and uses va_arg with a vector type is also dependent on the vector ABI.

Patch updated to address these cases as well.

See the minor comment inside. Otherwise, this LGTM now. Thanks!

Makes sense to me as well. The SystemZ changes are fine.

In D103865#2851145, @jonpa wrote:

Updated per review.

I was thinking of just using emitInt16 for the three words, that will show as .word in the textual assembler output, which may not be pretty but is certainly correct. (You could in addition use an emitRawComment to print a textual representation to make the file more readable).

Ah, ok, I did not realize at the time I could just print the instruction as numbers... I tried this instead now, since we don't really want to have a generic subtarget around. Is there any reason to print 3 x Int16? Would it be better to print it as one (or three) hex if possible?

In D103865#2848831, @jonpa wrote:

In D103865#2837530, @jonpa wrote:

I updated the patch to create the length-1 and trip count on the DAG instead.

The SystemZ part now looks good to me, except for the point made inline. It still would be good if the rest of the patch could be looked at by someone familiar with ARM. (Maybe it would make sense to split the patch two parts?)

LGTM.

LGTM

In D103865#2831600, @jonpa wrote:

One thing that is odd is the extra always-zero parameter to the LoopVarLen insns. This really should be removed.

Done - it was easy to remove on the MachineInstr, but on the SystemZISD node it seems reasonable to keep a dummy zero there and reuse the existing opcode, or?

This looks pretty good to me. One thing that is odd is the extra always-zero parameter to the LoopVarLen insns. This really should be removed.

I haven't looked at the code in detail yet, but just a couple of general comments:

• The GNU attribute support should probably just go into the generic ELF target streamer. This concept is supported in principle on all ELF targets. (That might also avoid the need for multiple inheritance.)
• If we support the GNU attribute generally in the ELF streamer, then we should also support it generally in the asm streamer (and then also the common asm parser, I guess)
• ASM parser support for .gnu_attribute should support any tag&value just like GAS does.

In D102046#2793430, @DavidSpickett wrote:

I have implemented that requires for Arm in https://reviews.llvm.org/D103512.

Perhaps the same could apply to s390x?

LGTM, thanks!

In D102615#2789611, @mkazantsev wrote:

Oh I see... That's nasty. In this case, could you please help me to reproduce it? I wasn't able to find the sources of this test in LLVM repository. IR dump before Loop Deletion would work too.

LGTM.

In D103343#2789521, @anirudhp wrote:

In D103343#2789121, @uweigand wrote:

Maybe it would be clearer to be explicit and write:

if (C.getTargetTriple().isSystemZ() && C.getTargetTriple().isOSzOS())

This should have the same effect

Yes, that would be the same behaviour.

but avoid the implicit assumptions.

Are we making an implicit assumption? In the getDefaultFormat function in Triple.cpp, we have:

case Triple::systemz:
  if (T.isOSzOS())
    return Triple::GOFF;

GOFF is only set when the arch is systemz and the os is zos.

In D102894#2786239, @jonpa wrote:

Patch updated - in progress.

This is probably not even necessary, since even a pointer to a vector would still trigger the emission of the gnu_attribute...
Yes, indeed. The alignment of 128-bit vector types is different between the two ABIs, which means that any use of such types, even via pointers or as struct elements, will trigger ABI differences.

Aha... I removed the function attribute emission from clang and now do all the work in SystemZAsmPrinter instead. Added a check for actually passed arguments to a vararg function.

For an example of how to handle such things, you might look at emitLocalEntry in the PowerPC target. You'll need a virtual function in TargetStreamer.h that can be called by the AsmPrinter.cpp code. Then there need to be two implementations of the virtual function, one for emitting assembler text, and one when directly emitting object files. Finally, you'll need to handle reading the directive in assembler code in the AsmParser.

I did an attempt at this except for the parser part, which seems to work.

Ah wait: the test case uses -fno-asynchronous-unwind-tables ! This explains why unwind cannot work - on s390x the ABI requires .eh_frame everywhere, and there is no alternative unwind mechanism via a frame pointer. The "fast unwinder" instead uses the (deprecated) "backchain" mechanism, but that is off by default and requires use of the -mbackchain option. That option is actually set by the various .lit.cfg files, but I guess the special invocation in this test case ignores those flags?

In D102046#2780372, @MaskRay wrote:

In D102046#2779171, @uweigand wrote:

In D102046#2767927, @uweigand wrote:

In D102046#2766553, @MaskRay wrote:

How about s/A<10>()/A<7>()/? Does that pass on s390x?

This passes, yes.

Can we make that change in mainline? I'd really like to get our build bots green again ...

Pushed 3a678fe3e29fe48d9b4efc936edd2ac43c720bae

Maybe it would be clearer to be explicit and write:

Looking at the assembly, only a single function shows any difference between the two versions: "interp" from MultiSource/Benchmarks/MallocBench/gs/interp.c.

In D102615#2788912, @mkazantsev wrote:

The last failure also looks like a timeout, I don't see any indication of miscompile:

FAIL: test-suite :: MultiSource/Benchmarks/MallocBench/gs/gs.test (73 of 2021)
******************** TEST 'test-suite :: MultiSource/Benchmarks/MallocBench/gs/gs.test' FAILED ********************

/home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/tools/timeit-target --limit-core 0 --limit-cpu 7200 --timeout 7200 --limit-file-size 104857600 --limit-rss-size 838860800 --append-exitstatus --redirect-output /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/MultiSource/Benchmarks/MallocBench/gs/Output/gs.test.out --redirect-input /dev/null --chdir /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/MultiSource/Benchmarks/MallocBench/gs --summary /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/MultiSource/Benchmarks/MallocBench/gs/Output/gs.test.time /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/MultiSource/Benchmarks/MallocBench/gs/gs -DNODISPLAY INPUT/large.ps
cd /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/MultiSource/Benchmarks/MallocBench/gs ; /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/tools/fpcmp-target /home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/MultiSource/Benchmarks/MallocBench/gs/Output/gs.test.out gs.reference_output

/home/uweigand/sandbox/buildbot/clang-s390x-linux-lnt/test/sandbox/build/tools/fpcmp-target: FP Comparison failed, not a numeric difference between '
' and 'g'

@uweigand could you please confirm that the output like this could have been a result of timeout?

Unfortunately, I now see this patch also caused another regression, this time in the s390x multistage builder. Again, this was initially hidden by other regressions; now that these are resolved, you can see the problems here (https://lab.llvm.org/buildbot/#/builders/8/builds/1277):
FAIL: LLVM-Unit::FixedPoint.FixedToFloat
FAIL: LLVM-Unit::FixedPoint.FloatToFixed
FAIL: Clang::fixed_point_compound.c
FAIL: Clang::fixed_point_conversions.c
FAIL: Clang::fixed_point_conversions_half.c
FAIL: Clang::fixed_point_conversions_const.c

This commit also introduced a regression in the s390x-lnt buildbot due to an apparent miscompilation of the test-suite::gs.test test case:
https://lab.llvm.org/buildbot/#/builders/45/builds/3264

Hmm, the https://reviews.llvm.org/D103288 commit did fix the problem on s390x again. Thanks!

I looks like this patch, when committed as 1c7f32334d4becc725b9025fd32291a0e5729acd, caused two additional build bot failures on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/3829
FAIL: libFuzzer:: msan.test
FAIL: libFuzzer:: swap-cmp.test
This was unfortunately hidden by other failures at the time, so I didn't notice earlier.

LGTM.

This version LGTM now. Thanks!

In D103077#2780921, @jonpa wrote:

In D103077#2779949, @fhahn wrote:

Adjust failing SystemZ test. It would be great if someone familiar with SystemZ could take a look to double-check this makes sense. Perhaps @jonpa?

Yes, this looks like a correct test update to me: the changed immediate operand reflects that now only two shifted bits are inserted into %r1 (instead of all 32).

I wonder what the rationale behind this patch is: if the instruction is poison to begin with, then the program is broken and I don't understand how changing the argument value can change that..?

Adding @uweigand as well just in case...

LGTM, thanks.

Function return value / parameters: A C function with a vector parameter will be lowered by Clang differently depending on the subtarget vector support. On z13 it may be a <4 x i32>, while on zEC12 it is instead <4 x i32>*. When running instruction selection there would be then no way of telling if <4 x i32>* was a pointer argument or if it was a software ABI vector variable argument. Therefore a function attribute is emitted for each function "has-vector-arg" where as applicable. I guess the alternative might be to actually do this during instruction selection while considering the subtarget vector support presence - if using clang (and not llc), this might be enough. (E.g. if subtarget has vector support, then <4 x i32>* is really a pointer...)