User Details
- User Since
- Sep 22 2017, 3:13 PM (173 w, 5 d)
Wed, Jan 13
Nov 2 2020
Perhaps those were some registers that passes by in being classified inside a feature. The usual behavior in llvm (at least for arm), if I'm not mistaken, is to not do so.
So, it LGTM.
Sep 30 2020
I'm not the right reviewer for this patch.
Aug 19 2020
I'm out in vacations, just have my phone here. I'm surprised I forgot to
preserve de debug information, sorry about that.
Aug 18 2020
Feel free to move on this patch, I'm in vacations.
Aug 3 2020
Jul 30 2020
Fixed true -> false for isSOImm when checking for reducing to t1 instructions
Ping ... ping...
Jul 24 2020
Ping
Jul 22 2020
Thanks. LGTM.
Jul 21 2020
Ok, this patch seems to be correct, but it would be nice to have a test.
You can use clang -mllvm -stop-before=arm-cp-islands -mllvm --simplify-mir to obtain a machine IR before the patch, and use llc -run-pass=arm-cp-islands to validate that the alignment for the function is set to 4.
Sorry, it seems I was looking the wrong instruction, it should be the label variant: vldr.16 s0, .LCPI0_0
Hi, thanks for working on this.
What exactly are you trying to fix? From what I see from https://developer.arm.com/docs/ddi0597/h/simd-and-floating-point-instructions-alphabetic-order/vldr-immediate-load-simdfp-register-immediate
VLDR.16 s0,{pc}+0x16 requires only a alignment of 2 bytes, as it has only a single zero appended in the case of half (.16):
T1 Half-precision scalar (size == 01) (Armv8.2) VLDR{<c>}{<q>}.16 <Sd>, [<Rn> {, #{+/-}<imm>}] esize = 8 << UInt(size); add = (U == '1'); imm32 = if esize == 16 then ZeroExtend(imm8:'0', 32) else ZeroExtend(imm8:'00', 32);
Jul 17 2020
Fixed remaining of inline comments
Indeed not all of them. Fixed this time.
Jul 15 2020
Ping
Jul 10 2020
Preserve name
Jul 9 2020
Re-fixed test file, now showing only differences
Clear users assumptions (and test it)
Jul 8 2020
Fixed test location and command
Jul 7 2020
Moved to BDCE.
Rebased.
Now the AAPCS explicitly avoids conflicts with the C11, by not imposing any restriction
when the natural container will overlap a zero lenght bit-field:
https://github.com/ARM-software/abi-aa/commit/2334fc7611ede31b33e314ddd0dc90579015b322
Both 32 and 64 bit versions were updated on the same way.
Rebase
Jun 12 2020
Perhaps we could move to making half a valid type for the arm back-end as follow up patches. Allowing half as argument through the IR is already a step to that direction.
IMO this patch is already quite big and it excels in fixing the bugs it proposed.
May 27 2020
Hi @rsmith,
are you still looking into this?
cheers
May 15 2020
Addressed requests
May 14 2020
LGTM, as far @rjmccall 's concern about documentation is addressed.
Would it be possible to pass a half argument and fix-it-up at CodeGenPrepare?
May 11 2020
From my point it does LGTM.
May 5 2020
Ping
I believe we can avoid creating some blocks for latter removing them, no?
May 1 2020
Hi, @ldionne
I just submitted a what I think is the test fix. It was the only place I was not using the second element of the string and could generate a underflow.
Apr 30 2020
Apr 29 2020
Hi @davide, do you have any plans to address this issue? Regards.
Apr 27 2020
Now testing only simple-register-coalescing
Fixed x86 test
Do not perform register coalescing
Apr 25 2020
Apr 20 2020
Ping. I would like to move along this small patch.
Apr 14 2020
Ping
Hi @rnk and @efriedma, indeed my initial thoughts were as well the register allocator, specially because using the pbqp allocator there are no spills for this example so it does not fail.
From what I see in the code it seems that none of the register allocators know anything about setjmp. There's an ancient llvm-dev post confirming it: https://lists.llvm.org/pipermail/llvm-dev/2011-October/043731.html
Apr 9 2020
LGTM, not forgetting to remove the exit comments.
Hi @efriedma,
thanks for looking into this.
Apr 8 2020
Apr 6 2020
Ping
Apr 2 2020
Ping
Apr 1 2020
Mar 31 2020
Mar 27 2020
Gentle Ping. I believe all issues with this fix have been fulfilled.
Mar 23 2020
- Fixed tests to use "\\b" instead of "\b".
Mar 18 2020
LGTM
Hi,
thanks for looking into this. The patch LGTM, but regarding the indentation, I don't know what would be the best practice here. We tend to like to preserve the line-git-history, but if we start ignoring the formater check, then it has no sense in they being here.
Perhaps @t.p.northover or @olista01 could share their thoughts here.
Mar 17 2020
Mar 16 2020
Ping.
Mar 13 2020
- Removed incorrect spacing changes
- Removed unecessary changes
- Do not allow a instruction that may load or store between the LdSt[SP] and the SP update.
Mar 12 2020
- Do not use pseudo-value
Hi @EricWF, thanks for the review.
Indeed there seems to have some other unrelated issue for matching the word boundary \b. Perhaps I wrongly assumed that it should also match line beginning and end.
For example, I would expect all these matches to succeed:
Mar 11 2020
- Correct mayAlias to isStack
- Fixed tests
- Avoid optimization when target is Windows with CFI, as we need to correctly update unwind information
Ping.
Mar 10 2020
LGTM, after a nit inline.
Mar 9 2020
Do not optimize post-increment SP in windows targets
Mar 8 2020
Mar 6 2020
Mar 5 2020
Is this missing a test?
LGTM with a nit: we can save some space using sintax like this:
let isLaneQ = 1 in def UDOT_LANEQ : SOpInst<"vdot_laneq", "..(<<)(<<Q)I", "iUiQiQUi", OP_DOT_LNQ>;
or concatenating those that are just one after the other:
let isLaneQ = 1 in { def VFMLAL_LANEQ_LOW : SOpInst<"vfmlal_laneq_low", "(F>)(F>)F(FQ)I", "hQh", OP_FMLAL_LN>; def VFMLSL_LANEQ_LOW : SOpInst<"vfmlsl_laneq_low", "(F>)(F>)F(FQ)I", "hQh", OP_FMLSL_LN>; def VFMLAL_LANEQ_HIGH : SOpInst<"vfmlal_laneq_high", "(F>)(F>)F(FQ)I", "hQh", OP_FMLAL_LN_Hi>; def VFMLSL_LANEQ_HIGH : SOpInst<"vfmlsl_laneq_high", "(F>)(F>)F(FQ)I", "hQh", OP_FMLSL_LN_Hi>; }