User Details
- User Since
- Apr 14 2013, 11:48 AM (404 w, 6 d)
Yesterday
Thu, Jan 14
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
Wed, Jan 13
LGTM
Tue, Jan 12
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.
Mon, Jan 11
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" ...
Dec 16 2020
LGTM, thanks!
Dec 15 2020
Dec 14 2020
This all looks good to me, the test case also looks fine (and the offset match what GCC is doing).
Dec 11 2020
OK, I think this is fine then.
Ah, OK . Looks good to me.
Dec 10 2020
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.
Dec 9 2020
I think it still makes sense to have the backchain store local in inlineStackProbe.
Dec 8 2020
I'm not sure I like the implicit assumptions the patch makes between inlineStackProbe and emitPrologue.
Nov 27 2020
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.)
Nov 24 2020
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.
Nov 23 2020
Oh, any another thing: we might also want to handle STRICT_FSETCCS in addition to STRICT_FSETCC.
Nov 18 2020
LGTM, thanks!
Nov 17 2020
Nov 9 2020
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:
Nov 4 2020
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.
Oct 28 2020
Oct 23 2020
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.
Oct 21 2020
See inline comment; otherwise this LGTM.
Oct 20 2020
Oct 16 2020
Oct 15 2020
@jonpa can you check whether the SystemZ test case you added still checks what it was intended to check here?
Oct 14 2020
Strange that this wasn't noticed earlier. In any case, patch LGTM.
Oct 9 2020
LGTM, thanks!
Oct 8 2020
Oct 6 2020
See inline comment. Otherwise LGTM.
Oct 5 2020
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.
Oct 2 2020
Oct 1 2020
Sep 30 2020
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()):
Sep 29 2020
LGTM now, thanks!
Sep 28 2020
Sep 25 2020
LGTM as well.
Sep 15 2020
This LGTM, thanks!
Sep 11 2020
Sep 9 2020
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.
Sep 8 2020
We're finally there, great :-) LGTM.
Makes sense to me. As an aside, this code:
This LGTM now. Thanks!
Sep 4 2020
LGTM now, thanks!
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?
Sep 2 2020
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.
Sep 1 2020
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!
Aug 31 2020
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.
Aug 28 2020
Aug 26 2020
Aug 21 2020
OK, this now looks all good to me as well. Thanks!
Aug 20 2020
A few comments inline, otherwise this looks good.
LGTM now. Thanks!
Aug 19 2020
This LGTM now. Thanks!
Aug 13 2020
Aug 12 2020
Yes, this makes sense to me as well. Patch LGTM.
Aug 10 2020
Aug 4 2020
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.
Aug 3 2020
This LGTM now.
I'm not sure what specific problem you're trying to solve here, but this change in itself looks incorrect.
Jul 30 2020
Sure, of course assuming the test still passes.
Jul 29 2020
LGTM for SystemZ as well.
Jul 24 2020
Jul 23 2020
Jul 22 2020
LGTM with the format fixes.
Jul 21 2020
This LGTM now.
Jul 17 2020
See inline comments about chain handling.
Jul 16 2020
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.