Page MenuHomePhabricator

uweigand (Ulrich Weigand)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 14 2013, 11:48 AM (427 w, 1 d)

Recent Activity

Fri, Jun 18

uweigand added a comment to D103865: [SystemZ] Generate XC loop for memset 0 of variable length..

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.

Fri, Jun 18, 5:59 AM · Restricted Project

Mon, Jun 7

uweigand added a comment to D102894: [SystemZ] Emit .gnu_attribute for an externally visible vector abi..

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.
Mon, Jun 7, 6:44 AM · Restricted Project

Wed, Jun 2

uweigand added a comment to D102046: [sanitizer] Fall back to fast unwinder.

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

Perhaps the same could apply to s390x?

Wed, Jun 2, 6:09 AM · Restricted Project

Mon, May 31

uweigand accepted D103343: [SystemZ][z/OS] Stricter condition for HLASM class instantiation.

LGTM, thanks!

Mon, May 31, 9:07 AM · Restricted Project
uweigand added a comment to D102615: [LoopDeletion] Break backedge if we can prove that the loop is exited on 1st iteration (try 3).

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.

Mon, May 31, 9:06 AM · Restricted Project
uweigand accepted D103320: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 2.

LGTM.

Mon, May 31, 7:56 AM · Restricted Project
uweigand added a comment to D103343: [SystemZ][z/OS] Stricter condition for HLASM class instantiation.

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.

Mon, May 31, 7:55 AM · Restricted Project
uweigand added a comment to D102894: [SystemZ] Emit .gnu_attribute for an externally visible vector abi..

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.

Mon, May 31, 6:46 AM · Restricted Project
uweigand added a comment to D102046: [sanitizer] Fall back to fast unwinder.

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?

Mon, May 31, 6:19 AM · Restricted Project
uweigand added a comment to D102046: [sanitizer] Fall back to fast unwinder.

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

Mon, May 31, 6:00 AM · Restricted Project
uweigand added a comment to D103343: [SystemZ][z/OS] Stricter condition for HLASM class instantiation.

Maybe it would be clearer to be explicit and write:

Mon, May 31, 2:12 AM · Restricted Project
uweigand added a comment to D102615: [LoopDeletion] Break backedge if we can prove that the loop is exited on 1st iteration (try 3).

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

Mon, May 31, 1:29 AM · Restricted Project
uweigand added a comment to D102615: [LoopDeletion] Break backedge if we can prove that the loop is exited on 1st iteration (try 3).

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?

Mon, May 31, 12:47 AM · Restricted Project

Sat, May 29

uweigand added a comment to D101806: [TargetLowering] Only inspect attributes in the arguments for ArgListEntry.

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

Sat, May 29, 9:12 AM · Restricted Project
uweigand committed rGc123c178b26e: [SystemZ] Set getExtendForAtomicOps to ISD::ANY_EXTEND (authored by uweigand).
[SystemZ] Set getExtendForAtomicOps to ISD::ANY_EXTEND
Sat, May 29, 3:17 AM

Fri, May 28

uweigand added a comment to D102615: [LoopDeletion] Break backedge if we can prove that the loop is exited on 1st iteration (try 3).

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

Fri, May 28, 6:45 AM · Restricted Project
uweigand added a comment to D101806: [TargetLowering] Only inspect attributes in the arguments for ArgListEntry.

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

Fri, May 28, 3:52 AM · Restricted Project

Thu, May 27

uweigand added a comment to D101806: [TargetLowering] Only inspect attributes in the arguments for ArgListEntry.

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.

Thu, May 27, 9:42 AM · Restricted Project

Wed, May 26

uweigand accepted D103111: [SystemZ][z/OS] Enable the AllowAtInName attribute for the HLASM dialect.

LGTM.

Wed, May 26, 3:10 AM · Restricted Project
uweigand accepted D100788: [SystemZ] Support i128 inline asm operands.

This version LGTM now. Thanks!

Wed, May 26, 3:09 AM · Restricted Project
uweigand added a comment to D103077: [DAGCombine] Poison-prove scalarizeExtractedVectorLoad..

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...

Wed, May 26, 3:05 AM · Restricted Project

Tue, May 25

uweigand accepted D103091: [SystemZ][z/OS] Validate symbol names for z/OS for printing without quotes.

LGTM, thanks.

Tue, May 25, 8:52 AM · Restricted Project
uweigand added a comment to D102894: [SystemZ] Emit .gnu_attribute for an externally visible vector abi..

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...)

Tue, May 25, 3:46 AM · Restricted Project
uweigand added a comment to D102046: [sanitizer] Fall back to fast unwinder.

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

This passes, yes.

Tue, May 25, 3:20 AM · Restricted Project
uweigand added a comment to D100788: [SystemZ] Support i128 inline asm operands.

Now it's looking mostly good to me; a couple of (mainly cosmetic) remaining comments inline.

Tue, May 25, 3:14 AM · Restricted Project
uweigand accepted D103057: [SystemZ] Return true from preferZeroCompareBranch().

Indeed, this looks like a win. LGTM.

Tue, May 25, 12:39 AM · Restricted Project

May 20 2021

uweigand accepted D102793: [SystemZ][z/OS] Implement getHostCPUName for z/OS.

LGTM, thanks!

May 20 2021, 9:11 AM · Restricted Project
uweigand added a comment to D100788: [SystemZ] Support i128 inline asm operands.

This already looks a lot simpler. I'm now wondering if it would be even better to change the getNumRegisters interface from (ValueVT, RC) to (ValueVT, RegisterVT). (Or I guess an Optional<RegisterVT> if we don't want to change all callers.) All callers of getNumRegisters already have the RegisterVT immediately available, and it allows the back end to make any distinction it needs just as well as a RC would.

May 20 2021, 9:01 AM · Restricted Project

May 19 2021

uweigand added a comment to D100788: [SystemZ] Support i128 inline asm operands.

Patch updated per review.

Can you elaborate? I thought InstrEmitter::EmitCopyFromReg() was supposed to also work with types that are not legal. It uses
SrcRC = TRI->getMinimalPhysRegClass(SrcReg, VT);
to get the RC for use with a physreg ...

It does that for SrcRC, but for DstRC it calls getRegClassFor(). Another way of solving this might be to check if the type is legal and if not then take SrcRC (see patch).

May 19 2021, 2:13 PM · Restricted Project
uweigand added a comment to D100788: [SystemZ] Support i128 inline asm operands.

So we should check the register that was created for the matching index (which is inAsmNodeOperands[CurOp+1] at this point); if it is a virtual register, get the RC from the flags, and if it is a physical register, use the RC this physreg is a member of. Does that make sense?

Yes... I however found that we have to implement getRegClassFor(MVT::Untyped) to make inline assembly phys-regs work in InstrEmitter::EmitCopyFromReg(). If we do this we can use this also in SelectionDAGBuilder instead of the above. This seems to work as long as we only have one untyped regclass even though it is slightly confusing to not use the regclass returned by getRegForInlineAsmConstraint().

May 19 2021, 4:45 AM · Restricted Project
uweigand added a comment to D96033: [clang-repl] Land initial infrastructure for incremental parsing.

Yes, this patch fixes the problem for me. Thanks!

May 19 2021, 2:13 AM · Restricted Project
uweigand added a comment to D102046: [sanitizer] Fall back to fast unwinder.

The new test case fails on s390x with "WARNING: Symbolizer buffer too small" from SymbolizerProcess::ReadFromSymbolizer, and it is indeed true that the full backtrace at this point does not fit in the default buffer size of 16 KB. In the innermost function we have 1024 instances of the string "std::vector<int, std::allocator<int> >" in the function type, which in itself is already 40 KB. Plus the higher stack frames as well ...

I can make it pass again by increasing the buffer size to 128 KB. But I'm not quite sure if that is the intended behavior here?

I copied the test code from ../symbolize_stack.cpp, so presumably that file may have a similar issue?
Both give me WARNING: Symbolizer buffer too small on my x86-64 Linux machine.

May 19 2021, 1:45 AM · Restricted Project

May 18 2021

uweigand accepted D101010: [SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ.

So it looks like this is good to go now. LGTM, thanks!

May 18 2021, 8:35 AM · Restricted Project
uweigand accepted D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.

OK, this looks good to me now. Thanks!

May 18 2021, 8:27 AM · Restricted Project
uweigand added a comment to D96033: [clang-repl] Land initial infrastructure for incremental parsing.

@hubert.reinterpretcast, thanks for the feedback. I have created a patch as discussed -- https://reviews.llvm.org/D102688

@uweigand, thanks for reaching out. I believe the patch above should fix your setup. Could you confirm?

May 18 2021, 8:05 AM · Restricted Project
uweigand added a comment to D102046: [sanitizer] Fall back to fast unwinder.

The new test case fails on s390x with "WARNING: Symbolizer buffer too small" from SymbolizerProcess::ReadFromSymbolizer, and it is indeed true that the full backtrace at this point does not fit in the default buffer size of 16 KB. In the innermost function we have 1024 instances of the string "std::vector<int, std::allocator<int> >" in the function type, which in itself is already 40 KB. Plus the higher stack frames as well ...

May 18 2021, 7:44 AM · Restricted Project
uweigand added a comment to D96033: [clang-repl] Land initial infrastructure for incremental parsing.

Looks like this is also failing on s390x:

May 18 2021, 4:11 AM · Restricted Project

May 14 2021

uweigand accepted D102370: [SystemZ] [z/OS] Add SystemZCallingConventionRegisters class.

LGTM, thanks!

May 14 2021, 3:23 AM · Restricted Project

May 12 2021

uweigand added inline comments to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
May 12 2021, 2:31 AM · Restricted Project

May 11 2021

uweigand added a comment to D102064: Parse vector bool when stdbool.h and altivec.h are included.

This means the implementation now deviates from the documented vector extension syntax, right? I guess it's strictly an extension of the documented syntax, but that may still lead to incompatibilities with other compilers for the platform. If we want to make such a change, should it be synchronized with e.g. GCC, XL, etc. ?

GCC and XL already accept this syntax on Linux on Power and AIX.

For example this simple test case:

#include <stdbool.h>
#include <altivec.h>

vector bool char bc;

Can compile with GCC 9/10 and XLC 16.1 on Linux on Power. On AIX, GCC 8.3 on AIX and XLC 16.1 can also compile it successfully. Latest main trunk clang throws up an error on those platforms.

From offline conversation it looks like XLC on z/OS can also compile the test case. @Everybody0523 can confirm for sure.

May 11 2021, 10:09 AM · Restricted Project, Restricted Project
uweigand added a comment to D102064: Parse vector bool when stdbool.h and altivec.h are included.

This means the implementation now deviates from the documented vector extension syntax, right? I guess it's strictly an extension of the documented syntax, but that may still lead to incompatibilities with other compilers for the platform. If we want to make such a change, should it be synchronized with e.g. GCC, XL, etc. ?

May 11 2021, 9:30 AM · Restricted Project, Restricted Project
uweigand added inline comments to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
May 11 2021, 8:29 AM · Restricted Project
uweigand added inline comments to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
May 11 2021, 4:04 AM · Restricted Project
uweigand added inline comments to D100788: [SystemZ] Support i128 inline asm operands.
May 11 2021, 2:05 AM · Restricted Project

May 7 2021

uweigand added inline comments to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
May 7 2021, 7:53 AM · Restricted Project

May 6 2021

uweigand accepted D101993: [SystemZ] Don't use libcall for 128 bit shifts.

Thanks, this LGTM. We can always add further improvements later if necessary.

May 6 2021, 6:55 AM · Restricted Project

May 5 2021

uweigand accepted D101897: [SystemZ] Support builtin_frame_address with packed stack and no backchain..

LGTM, thanks!

May 5 2021, 6:08 AM · Restricted Project
uweigand added inline comments to D100788: [SystemZ] Support i128 inline asm operands.
May 5 2021, 4:35 AM · Restricted Project
uweigand added inline comments to D100788: [SystemZ] Support i128 inline asm operands.
May 5 2021, 3:17 AM · Restricted Project

May 3 2021

uweigand accepted D101665: [SystemZ][z/OS] Enforce prefix-less registers in SystemZAsmParser for the HLASM dialect..

LGTM, thanks!

May 3 2021, 4:20 AM · Restricted Project

Apr 28 2021

uweigand added inline comments to D101010: [SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ.
Apr 28 2021, 5:55 AM · Restricted Project
uweigand added inline comments to D101010: [SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ.
Apr 28 2021, 5:03 AM · Restricted Project

Apr 27 2021

uweigand accepted D101308: [SystemZ][z/OS] Remove register prefixes when printing out the register..

LGTM as well.

Apr 27 2021, 5:47 AM · Restricted Project

Apr 23 2021

uweigand accepted D101053: [doc] Clarify constrained fcmps behavior.

B.t.w. even if the raised exception does cause an actual trap, the result of the instruction might _still_ be accessed. This is now of course platform-specific, but e.g. on Linux it is possible to inspect the result of the instruction in the trap handler if one is installed; it is also possible for the trap handler to resume execution so that the result may also still be used by the original code afterwards. In those cases, even if there is a trap, there would still be difference between fcmp and fcmps visible.

Apr 23 2021, 3:21 AM · Restricted Project

Apr 22 2021

uweigand added inline comments to D101010: [SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ.
Apr 22 2021, 11:00 AM · Restricted Project
uweigand added a comment to D101053: [doc] Clarify constrained fcmps behavior.

The statement is now correct, but it's still not completely clear to me why it is necessary. All floating-point intrinsics may still produce a result in addition to raising an exception (if they do), so the definition of the result is always independent of the question what -if any- exceptions are raised. Why does this need to be called out here specifically?

Apr 22 2021, 9:49 AM · Restricted Project
uweigand added inline comments to D101010: [SystemZ] [z/OS] Add XPLINK64 Calling Convention to SystemZ.
Apr 22 2021, 9:40 AM · Restricted Project
uweigand added a comment to D101053: [doc] Clarify constrained fcmps behavior.

I don't believe this change is correct. Note that even if an exception is raised, the instruction may still have a result (if exceptions are configured to only set a flag bit and not actually trap). This result will still be different for ordered vs. unordered comparisons.

Apr 22 2021, 5:52 AM · Restricted Project

Apr 13 2021

uweigand accepted D99891: [SystemZ][z/OS] Introduce dialect querying helper functions.

LGTM.

Apr 13 2021, 3:16 AM · Restricted Project

Apr 6 2021

uweigand added a comment to D97514: [SystemZ] Assign the full space for promoted and split outgoing args.

Thanks for the heads-up, @luismarques ! However, this is not really an issue on SystemZ as all arguments in question have an alignment requirement of 8 bytes on our platform.

Apr 6 2021, 7:51 AM · Restricted Project

Mar 15 2021

uweigand accepted D97901: [SystemZ] Test for infinity in testFPKind()..

LGTM, thanks!

Mar 15 2021, 1:40 PM · Restricted Project

Mar 12 2021

uweigand added inline comments to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
Mar 12 2021, 9:14 AM · Restricted Project
uweigand added inline comments to D98276: [AsmParser][SystemZ][z/OS] Introducing HLASM Parser support to AsmParser - Part 1.
Mar 12 2021, 12:31 AM · Restricted Project

Mar 11 2021

uweigand added a comment to D97975: [libFuzzer] add attribute noinline on Fuzzer::ExecuteCallback().

I don't think there is a s390x platform-specific problem involved here at all, so I'm not sure disabling the test for the platform is the right fix.

Mar 11 2021, 3:18 AM · Restricted Project

Mar 4 2021

uweigand accepted D97748: [SystemZ][z/OS] Add support to validate a HLASM Label..

LGTM, thanks.

Mar 4 2021, 7:13 AM · Restricted Project
uweigand added inline comments to D97901: [SystemZ] Test for infinity in testFPKind()..
Mar 4 2021, 3:55 AM · Restricted Project

Mar 3 2021

uweigand accepted D97604: [SystemZ] Reimplement the 1-byte compare-and-swap logic.

Ahh, you're right. It's done in common code by the default ATOMIC_CMP_SWAP_SUCCESS expander -- but we're not using that since we use our own custom expander! So it indeed has to be done there in the back end.

Mar 3 2021, 11:05 AM · Restricted Project
uweigand added a comment to D97604: [SystemZ] Reimplement the 1-byte compare-and-swap logic.

The change in lowerATOMIC_CMP_SWAP should be removed now. Otherwise this LGTM.

Mar 3 2021, 1:16 AM · Restricted Project

Mar 2 2021

uweigand added inline comments to D97748: [SystemZ][z/OS] Add support to validate a HLASM Label..
Mar 2 2021, 12:53 AM · Restricted Project
uweigand added a comment to D97604: [SystemZ] Reimplement the 1-byte compare-and-swap logic.

I added an AND to zero-out the high bits to perform the zero-extension from the narrow type. It seemed that if the source was a constant (e.g. '1'), the DAG.getNode(ISD::AND, ...) call folded the AND on the fly. And if the source was a parameter with the 'zeroext' attribute (or rather any result with an AssertZext node) ,the AND goes away during DAGCombine2. So for what I could see, there is not much extra work to do here.

Mar 2 2021, 12:47 AM · Restricted Project
uweigand accepted D97514: [SystemZ] Assign the full space for promoted and split outgoing args.

LGTM, thanks!

Mar 2 2021, 12:27 AM · Restricted Project

Mar 1 2021

uweigand accepted D97581: [SystemZ] Introduce distinction between the jg/jl family of mnemonics for GNU as vs HLASM.
  • Improved the implementation of MnemonicCondBranch by abstracting away details from SystemZInstrInfo.td
  • For this, a multi-class had to be used.
Mar 1 2021, 9:06 AM · Restricted Project
uweigand added inline comments to D97514: [SystemZ] Assign the full space for promoted and split outgoing args.
Mar 1 2021, 5:51 AM · Restricted Project
uweigand added a comment to D97604: [SystemZ] Reimplement the 1-byte compare-and-swap logic.

CmpVal: Should not need a LL[HC]R, as it should already be zero-extended also in the case of a non-constant, or?

Mar 1 2021, 5:20 AM · Restricted Project
uweigand added a comment to D97581: [SystemZ] Introduce distinction between the jg/jl family of mnemonics for GNU as vs HLASM.

Looks good in general. Just as a cosmetic issue, it would be nicer to be able to simply write:

Mar 1 2021, 4:05 AM · Restricted Project

Feb 26 2021

uweigand accepted D94250: [SystemZ] Introducing assembler dialects for the Z backend.

Looks like resolving the general issues with the AsmDialect setting is more complicated than I thought and may still take a while.

Feb 26 2021, 4:23 AM · Restricted Project
uweigand added a comment to D97514: [SystemZ] Assign the full space for promoted and split outgoing args.

I'm not sure if "getTypeToTransformTo(*DAG.getContext(), OrigArgVT)" results in the same type that is used by common code in all cases.

Feb 26 2021, 4:12 AM · Restricted Project

Feb 19 2021

uweigand accepted D96887: [SystemZ/z/OS] Initial changes to add the XPLink calling convention to tablegen.

LGTM, thanks!

Feb 19 2021, 5:39 AM · Restricted Project

Feb 18 2021

uweigand accepted D96568: [CFE, SystemZ] Emit s390.tdc instrincic for __builtin_isnan in Constrained FP mode..

LGTM as well, thanks!

Feb 18 2021, 2:42 AM · Restricted Project

Feb 17 2021

uweigand accepted D96867: [SystemZ] Separate LoZ ELF specifics in tablegen.

LGTM, thanks!

Feb 17 2021, 8:04 AM · Restricted Project

Feb 12 2021

uweigand added a comment to D96568: [CFE, SystemZ] Emit s390.tdc instrincic for __builtin_isnan in Constrained FP mode..

That's interesting. I presume that can be used to implement isinf as well? Perhaps better call the hook fpclassify or similar?

Feb 12 2021, 3:43 AM · Restricted Project

Feb 11 2021

uweigand added a comment to D96471: [SystemZ] Fix vecintrin.h to not emit alignment hints in vec_xl/vec_xst..

It's not a good idea to use the library routine "memcpy" here, because the name in theory be redefined by a user before including the header, and also there may not be a prototype in scope causing warnings when building with -Wall.

Feb 11 2021, 10:53 AM · Restricted Project

Feb 9 2021

uweigand added inline comments to D95444: Allow GNU inline asm using target-specific dialect variant.
Feb 9 2021, 1:46 AM · Restricted Project

Feb 8 2021

uweigand added a comment to D95444: Allow GNU inline asm using target-specific dialect variant.

At least for our use case, this is not relevant. We want to use the "GCC style asm" whenever compiling for the z/OS target, and the "HLASM style asm" whenever compiling for the Linux target. These are incompatible anyway (different ABI, different target OS), so they cannot be combined using LTO. We simply need to use asm dialects since both targets are implemented in a single SystemZ back-end. (While the OS and ABI are different, the ISA is of course the same, so it does not make sense to use a different back-end.)

@uweigand, its the other way right? We want to use the "GNU style asm" when compiling for the Linux target (on the Z backend) and use the "HLASM style asm" when compiling for the z/OS target (on the Z backend).

Feb 8 2021, 10:02 AM · Restricted Project
uweigand added a comment to D95444: Allow GNU inline asm using target-specific dialect variant.
In D95444#2546235, @rnk wrote:

Yes, sorry, I meant the assembly dialect that the GNU binutils assembler parses. What I'm trying to get at is that you might want to support separate blobs that use different dialects:

void foo() { asm volatile ("gnu style asm"); }
void bar() { asm volatile ("HLASM style asm"); }

If there is one global setting for the inline asm dialect, this won't work. You could create a flag to control the setting, but then if you want to use LTO on two objects that use different assembly dialects, the command line flag isn't sufficient. To fully address this use case, you would want to put the dialect on each inline assembly blob, similar to the way we handle the existing intel/gnu dialect flag.

Feb 8 2021, 3:45 AM · Restricted Project

Jan 26 2021

uweigand added a comment to D82862: [ThinLTO] Always parse module level inline asm with At&t dialect.
In D82862#2513044, @rnk wrote:

So why do you want GNU inline asm for clang-cl anyway? I thought the whole point of clang-cl was to be compatible with the Microsoft Visual Studio compiler, which I understand only supports the MS asm syntax?

We have users, in this case, I think it's V8, who would prefer to use gcc-style module level assembly if it is available. Their motivation is somewhat arbitrary, but generally, clang-cl supports a variety of extensions, some inherited from GCC, in all modes. Part of the point of switching compilers from MSVC to clang is to get access to those extensions.

Jan 26 2021, 7:12 AM · Restricted Project, Restricted Project
uweigand requested review of D95444: Allow GNU inline asm using target-specific dialect variant.
Jan 26 2021, 7:09 AM · Restricted Project

Jan 21 2021

uweigand added a comment to D82862: [ThinLTO] Always parse module level inline asm with At&t dialect.

The motivation for my change was really just to make ThinLTO compiles work the same as non-ThinLTO ones.

Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a bug. I wasn't aware that Clang and GCC's -masm= flags behaved differently in that way, but that certainly suggests there's a problem here.

So I'm wondering, if I remove the above setAssemblerDialect line and revert this commit, we should have inline asm (both module-level and GNU function-leve) respect the target-selected asm dialect variant both for ThinLTO and non-ThinLTO, so they should match again. Would that also solve the problem you were originally tracking?

Not completely, because clang-cl sets -x86-asm-syntax=intel to enable intel-style asm in assembly listing output. We'd have to find another way of doing that without affecting the inline asm dialect.

Jan 21 2021, 9:36 AM · Restricted Project, Restricted Project
uweigand committed rGb3a5abcb3696: [NFC][Doc] Mention SystemZ supports StackMap generation (authored by uweigand).
[NFC][Doc] Mention SystemZ supports StackMap generation
Jan 21 2021, 9:31 AM
uweigand closed D95040: [NFC][Doc] Mention SystemZ supports StackMap generation.
Jan 21 2021, 9:31 AM · Restricted Project

Jan 20 2021

uweigand accepted D95040: [NFC][Doc] Mention SystemZ supports StackMap generation.

LGTM, thanks!

Jan 20 2021, 6:23 AM · Restricted Project

Jan 15 2021

uweigand added a comment to D82862: [ThinLTO] Always parse module level inline asm with At&t dialect.

What is the reason for treating this differently in LLVM?

I'm not sure if it is related to this. I think one difference is that LLVM is supporting MS inline assembly. Although it uses Intel dialect, it has different representation in input, output, clobber etc. with GCC'.

Jan 15 2021, 3:51 AM · Restricted Project, Restricted Project

Jan 14 2021

uweigand added a comment to D94250: [SystemZ] Introducing assembler dialects for the Z backend.

Just a quick comment that I'm looking at this, but before approval I want to resolve the issues described in the comment in front of getMAIAssemblerDialect. See also discussion here: https://reviews.llvm.org/D82862

Jan 14 2021, 7:17 AM · Restricted Project
uweigand added a comment to D82862: [ThinLTO] Always parse module level inline asm with At&t dialect.

Hi @hans , we're having some issues with using the AssemblerDialect mechanism to support both the GNU assembler and the IBM HLASM assembler in the SystemZ back-end. See also the discussion started here: https://lists.llvm.org/pipermail/llvm-dev/2020-November/146875.html

Jan 14 2021, 7:15 AM · Restricted Project, Restricted Project

Jan 13 2021

uweigand accepted D94383: [SystemZ] Don't crash with -misched-cutoff.

LGTM

Jan 13 2021, 1:07 AM · Restricted Project

Jan 12 2021

uweigand added a comment to D94383: [SystemZ] Don't crash with -misched-cutoff.

I don't think we need to bother with skipping advancing the hazard state. I believe the main point of the cutoff is to avoid combinatorial explosion where there are many instructions to schedule and at each step there are many candidates to consider. Advancing the hazard state doesn't consider candidates and is therefore just a linear pass over instructions.

Jan 12 2021, 2:00 AM · Restricted Project

Jan 11 2021

uweigand added a comment to D94383: [SystemZ] Don't crash with -misched-cutoff.

Could you elaborate why initPolicy is the correct place to clear the Available list? I'm wondering because the default implementation doesn't appear to do that either, it looks like common code only clears the list in the main "init" ...

Jan 11 2021, 5:29 AM · Restricted Project

Dec 16 2020

uweigand accepted D93388: [Doc][SystemZ] Add Linux/SystemZ to Getting Started guide..

LGTM, thanks!

Dec 16 2020, 7:25 AM · Restricted Project

Dec 15 2020

uweigand committed rGebef92169ca5: [SystemZ] Remove most hard-coded R1D instances for sibcalls (authored by uweigand).
[SystemZ] Remove most hard-coded R1D instances for sibcalls
Dec 15 2020, 7:32 AM

Dec 14 2020

uweigand accepted D93171: [SystemZ] Improve handling of backchain offset.

This all looks good to me, the test case also looks fine (and the offset match what GCC is doing).

Dec 14 2020, 9:23 AM · Restricted Project