- User Since
- Apr 11 2019, 6:15 AM (76 w, 3 d)
Tue, Sep 15
Tue, Sep 8
I don't know too much about statepoints or GC, but you have addressed all of @reames comments, and the AArch64 parts and code style look correct to me, so LGTM.
Mon, Sep 7
Aug 25 2020
Aug 7 2020
Aug 3 2020
This test case has been failing on many of the ARM and AArch64 bots all weekend, e.g. http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/19408, would you mind looking into this?
Jul 21 2020
Jul 17 2020
You haven't addressed my earlier inline comments.
Jul 16 2020
Jul 14 2020
Jul 9 2020
This was blocked on D76291 for a while, but that is now committed, so I'll re-land this today.
Jul 8 2020
Jul 6 2020
Jul 2 2020
Jun 24 2020
That sounds very fragile - I can easily imagine future, unrelated changes removing that COPY, or emitting one in the case which doesn't currently have one, which would completely change the effect of this patch.
Jun 18 2020
only if the register holding function's address got spilled
Jun 17 2020
I don't think it makes sense to make f16 legal for targets which don't have any arithmetic operations on it, since that would be contrary to the definition of "legal". I'd also expect doing so to introduce a lot more complexity than this patch.
Jun 16 2020
I'd expect to see some extra checks to decide if reverting the indirect-call optimisation is profitable or not. If the loaded address is used more than three times, without being spilled, then I think it's better to leave the calls as indirect ones. If you don't think this is ever likely to be worthwhile for thumb 1, then it would be better to modify ARMTargetLowering::LowerCall to not do the transformation in the first place.
Jun 15 2020
Jun 12 2020
Jun 10 2020
I think we could make this conditional based on whether the caller has BTI enabled, because:
- If the caller has BTI enabled, then we correctly use X16 for a BTI callee, or wastefully (but still with correct behaviour) use X16 for a non-BTI callee.
- If the caller has BTI disabled, then it must be in a page with BTI disabled, and so is able to use BR with any register, even if the destination is BTI-protected.
LGTM, with some comments which can be addressed later given the time-sensitivity of this.
LGTM. Maybe we should rename MQPR and DPR_VFP2 since they're not really specific to those architectures, but obviously not in this patch.
Is this expected to work for the soft-float calling convention, or is clang still passing half-precision values as integer types for that? If the former, then this needs some tests for that case.
LGTM, thanks for working on this patch series, this looks much clearer than doing it in clang.
Jun 8 2020
I have the same concern as in D81402: will this still work if the call to the thunk goes through a long-branch veneer or PLT, which could clobber x16 and x17?
LGTM, inline comment could be done as a follow-up patch given that this is time-sensitive (recently published security vulnerability).
A few nits, but given the time sensitivity of this the only blocking one is how this will work with PLTs or long-branch veneers,
The pre-commit test failures are in the newly added test, so need to be fixed before this can be committed. My inline comments could be addressed as follow up patches given that this is related to a recently-published security vulnerability.
Since this patch adds loads, stores, moves and the calling convention, couldn't those be tested here, without waiting for intrinsics?
Jun 4 2020
This is causing the 32-bit ARM bots to fail test tools/obj2yaml/ELF/program-headers.yaml. It looks like an out of memory error, but it has been happening consistently for a large number of builds now, and looks related to this change.
May 28 2020
May 27 2020
May 26 2020
Should poly128_t be available on AArch32 too? I don't see anything in the ACLE version you linked restricting it to AArch64 only, and the intrinsics reference has a number of intrinsics available for both ISAs using it.
May 22 2020
May 21 2020
May 20 2020
May 19 2020
- Testing that the expression is the subtraction of two SymbolRefs seems needlessly restrictive, could we accept any expression, and reject it in the AsmBackend if it can't be fully resolved or converted to a relocation?
- Does this check that the value fits into a 16 bit immediate? The GNU assembler also seems to accept expressions which fit into a MOVZ/MOVN with a shift, though I can't see that being very useful for expressions involving symbol addresses.
- This needs tests.
Apr 22 2020
Actually, if the destination is XZR, the adds won't have any effect, so surely we don't need to emit any instructions? Could we just early-return without emitting anything if DestReg == XZR?
Committed as 5acafc540ad61c4cab90fbe3323c795d5210b72f.
Apr 20 2020
It would be nice to also print the mnemonic as a comment on the hint instruction, as these will commonly show up in compiled code. It looks like AArch64InstPrinter has access to a CommentStream which could be used for this.
Apr 9 2020
I think the failure was a bug in the machine verifier: D77783.
Apr 6 2020
Apr 2 2020
Thanks for the reproducer, I'll look into this.
Apr 1 2020
This is causing a compile failure on the 32-bit ARM bots:
Mar 31 2020
Mar 30 2020
Mar 25 2020
Mar 23 2020
It looks like the issues this is warning about isn't the .debug_line section, but the DW_AT_low/high_pc attributes of the compilation unit DIE. With DWARF3 we can emit a DW_AT_ranges attribute instead, but that didn't exist in DWARF2.
Mar 19 2020
Committed as rG6739805e24
Mar 18 2020
I agree with @dnsampaio here, it's better to match the existing style, and avoid irrelevant churn in the git history.
- I managed to reproduce the lsl-zero.s test failure on windows, this turned out to be because the test merges stdout and stderr, which can result in them being interleaved in ways which breaks tests. The test doesn't check anything in stderr, so fixed by redirecting that to /dev/null.
- The unit test was failing when built with MSVC, fixed by explicitly using UTF-8 string literals.
Mar 17 2020
See D76291 for that split-put patch.
@thakis, thanks for reverting, sorry I wasn't more proactive about that yesterday.
Mar 16 2020
My reply by email doesn't seem to have made it into phab, so repeating here (sorry for spamming everyone who did get the email):
The pre-commit test failure is also failing on the buildbots, not related to this patch.