In D82862#2500038, @pengfei wrote:

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

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

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

LGTM

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.

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

LGTM, thanks!

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

OK, I think this is fine then.

Ah, OK . Looks good to me.

This suggestion wasn't for performance reasons, but correctness. In theory, when using the backchain, it should be updated on every change to %r15 so that %r15 at all times points to a valid backchain value. Otherwise, unwinding using the backchain will randomly fail when starting at some PC where the stack pointer has been updated but the backchain not yet written.

It doesn't seem right to bail just for one non-prefetched access, so it seems reasonable to allow a relatively very small amount of non-prefetched instructions, to make the heuristic more stable. This patch suggests allowing 1 non-prefetched memory access per 32 prefetched ones. This handles LBM, and also gives two prefetches to imagick which however do not seem to play a role.

I think it still makes sense to have the backchain store local in inlineStackProbe.

I'm not sure I like the implicit assumptions the patch makes between inlineStackProbe and emitPrologue.

LGTM, thanks!

This looks good to me. The only thing I'm wondering is whether we shouldn't complete the NOP set by adding a 6-byte "jgnop". (I guess this is called jlnop with HLASM, but in order to keep with the jg* naming scheme in GAS we probably ought to use jgnop here.)

LGTM, thanks!

OK, I see. I guess the patch LGTM then with the two issues fixed.

See the two inline comments about STRICT_FSETCCS. I guess that shows this path is not really exercised right now -- maybe it would be a good idea to add some tests.

Oh, any another thing: we might also want to handle STRICT_FSETCCS in addition to STRICT_FSETCC.

LGTM, thanks!

In D91120#2399776, @spatel wrote:

It seems strange that we would need to add a TLI hook to avoid regressing a target that has abs/nabs support in the ISA.
If I'm seeing it correctly, SystemZ relies on ISD::ABS being expanded and then pattern-matching the expansion back to lpgr for example. Could we change that target to declare ISD::ABS as legal and adjust the tablegen matching for it?

I'm not sure whether the call instruction must match the declaration w.r.t. extension attributes. In the IR generated by clang, the two always match:

This doesn't look quite right to me. ldexp has a signed "int" argument, so you always must use the "sext" sign extension attribute on that argument.

In D90231#2358421, @jonpa wrote:

I think it makes general sense to not optimize the argument to @llvm.is.constant.

Note: The python tests show up as UNSUPPORTED when I try to run them with llvm-lit. I could not find out how to enable them.

See inline comment; otherwise this LGTM.

In D87279#2334510, @jonpa wrote:

The problem seems to be with a tied earlyclobber operand:

asm("" : "+&r"(a));

Per the comment in InlineAsm::ConstraintInfo::Parse(), only output can be earlyclobber.

I am not sure if the earlyclobber ('&') should with this patch be removed from the added use of the register, or if this has to for some reason really be tied to the def?

@jonpa can you check whether the SystemZ test case you added still checks what it was intended to check here?

Strange that this wasn't noticed earlier. In any case, patch LGTM.

LGTM, thanks!

See inline comment. Otherwise LGTM.

In D88817#2313657, @grimar wrote:

In D88817#2311572, @uweigand wrote:

It looks like this will trigger a warning being emitted on every use of llvm-readobj on a EM_S390 object file, which is probably not what we want. (They all also contain .gnu.hash, so the fact that .hash is ignored is not anything that the user should be concerned about. -- This warning may just cause unnecessary confusion, or parse error with automated tools etc.)

I'd like to wait for opinions of @jhenderson/@MaskRay to confirm them are happy to silently ignore the .hash section on EM_S390/EM_ALPHA platforms. It might probably
be confusing to see a .hash section in an object and no output with --hash-table.

It looks like this will trigger a warning being emitted on every use of llvm-readobj on a EM_S390 object file, which is probably not what we want. (They all also contain .gnu.hash, so the fact that .hash is ignored is not anything that the user should be concerned about. -- This warning may just cause unnecessary confusion, or parse error with automated tools etc.)

See the inline question about vector registers. This does not affect the current patch however, so this should be good to go in. LGTM.

In D88561#2305667, @grimar wrote:

In D88561#2305660, @uweigand wrote:

I see. In this case this patch doesn't really address PR47681, but only replaces a crash with wrong output ... Still preferable to not crash, but of course we still need to fix the root cause (at the least, by ignoring the .hash section if the entry size doesn't match).

The fix for PR47681 (s390 specific) is independent from the crash (all platforms), so it should be fixed separatelly.
I'll am going to prepare a follow-up to fix the PR47681 after landing this.

In D88561#2305598, @grimar wrote:

In D88561#2304127, @uweigand wrote:

I think the handling of the GNU hash table is actually already fully correct. The crash (or error message when using this patch) is simply a side effect of the fact that size of the DynSymRegion was clobbered here (near the end of ELFDumper<ELFT>::parseDynamicTable()):

DynSymRegion->Size = HashTable->nchain * DynSymRegion->EntSize;

This in turn is because HashTable->nchain (like all of HashTable) is just incorrect on s390x at the moment, as discussed in https://bugs.llvm.org/show_bug.cgi?id=47681

That is true for you executable. But this patch fixes a bit different case. Here I am creating the .gnu.hash table with a broken value in the buckets array.
Currently with the test case provided we crash an all platforms at the same place where your executable crashes too.
So with this patch we fix the crash, but not the reason of PR47681, which is s390x specific.

I think the handling of the GNU hash table is actually already fully correct. The crash (or error message when using this patch) is simply a side effect of the fact that size of the DynSymRegion was clobbered here (near the end of ELFDumper<ELFT>::parseDynamicTable()):

LGTM now, thanks!

LGTM as well.

This LGTM, thanks!

I'd like to make some progress on this soon, given that this still causes every LNT run on the SystemZ build bot to fail ...

Ping.

We're finally there, great :-) LGTM.

Makes sense to me. As an aside, this code:

This LGTM now. Thanks!

LGTM now, thanks!

In D87130#2256441, @jonpa wrote:

In D87130#2256173, @uweigand wrote:

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

I may be missing something, but as long as the memoization does a true intersection of the flags per this patch, there should not necessarily be anything broken, or?

In D86871#2252248, @spatel wrote:

In D86871#2249833, @uweigand wrote:

Thanks for the links, @spatel ! It seems to me that the change in "D46854 - changed code in SelectionDAGBuilder::visit()" is actually not correctly respecting the shared flags semantics. The node returned from getNodeForIRValue, while implementing the translation of this IR, might at the same be be used elsewhere (due to DAG node merging). If that other place does not allow FMF semantics, the flags in the DAG node can still be "undefined", and therefore the new code in "visit" will just replace the flags with a version appropriate only for this IR (which may allow FMF).

Am I missing something here? More generally, how can any after-the-fact setFlags ever be correct, given that the node might have already been merged?

That does seem wrong given the Flags.isDefined() check sitting within intersectWith(). I'm not seeing any regression test failures if we remove that entirely. So let's do that instead of only pushing it below the nsw/nuw intersects?

The algorithm used in the unsigned case isn't strict-safe. However, this is exactly the same algorithm that is also used in TargetLowering::expandFP_TO_UINT, where is has been made properly strict-safe in the meantime.

In D86963#2250247, @fhahn wrote:

Could you share the IR before DAGcombine that triggers the problem? DAGCombine already has limits in a few places that should prevent store merging from causing an explosion in compile-time. Would be good to know where this case slips through.

Thanks for the links, @spatel ! It seems to me that the change in "D46854 - changed code in SelectionDAGBuilder::visit()" is actually not correctly respecting the shared flags semantics. The node returned from getNodeForIRValue, while implementing the translation of this IR, might at the same be be used elsewhere (due to DAG node merging). If that other place does not allow FMF semantics, the flags in the DAG node can still be "undefined", and therefore the new code in "visit" will just replace the flags with a version appropriate only for this IR (which may allow FMF).

This LGTM now, thanks!

Interesting. I agree that the flags should always be reset here. In fact, I'm questioning why we should have that "isDefined" logic in the first place. It seems to me that any node with "undefined" flags can only be used correctly if any setting of the flags would be semantically correct -- but then we may just as well use the all-zero setting of the flags anyway.

OK, this now looks all good to me as well. Thanks!

A few comments inline, otherwise this looks good.

LGTM now. Thanks!

This LGTM now. Thanks!

Yes, this makes sense to me as well. Patch LGTM.

In D81537#2207249, @qiucf wrote:

In D81537#2193216, @uweigand wrote:

This doesn't look correct. As far as I can see, none of the conversion functions were actually changed to handle strict operations. For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.

The patch only adds a strict variant of the direct move, which seems to me the only operation where actually a strict version is not required ...

Thanks for pointing them out! I have something unclear about chains:

(1) If a constrained operation is expanded into several FP nodes a-b-c, they should all have chain set to former operation (b's chain is a, c's chain is b) even if they have def relationship?

This doesn't look correct. As far as I can see, none of the conversion functions were actually changed to handle strict operations. For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.

This LGTM now.

I'm not sure what specific problem you're trying to solve here, but this change in itself looks incorrect.

Sure, of course assuming the test still passes.

LGTM for SystemZ as well.

LGTM with the format fixes.

This LGTM now.

See inline comments about chain handling.

The one issue I still see is that when falling back to library calls on older platforms, signaling and non-signaling compares both are treated the same. But that's really a pre-existing problem because this is not fully supported by libgcc (see the comment in TargetLowering::softenSetCCOperands), so it's not really a problem with this patch.