I'm out in vacations, just have my phone here. I'm surprised I forgot to
preserve de debug information, sorry about that.

Feel free to move on this patch, I'm in vacations.

Fixed true -> false for isSOImm when checking for reducing to t1 instructions

Ping ... ping...

Ping

Thanks. LGTM.

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

Fixed remaining of inline comments

Indeed not all of them. Fixed this time.

Ping

Preserve name

Re-fixed test file, now showing only differences

In D60413#2141645, @spatel wrote:

In D60413#2139023, @dnsampaio wrote:

Fixed test location and command

The file is not moved here in the review, so something may be out-of-sync. The best thing would be to commit that file first with the current CHECK lines, then update it after applying this code patch. That way, we will highlight the test diffs.

Clear users assumptions (and test it)

Fixed test location and command

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

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.

Hi @rsmith,
are you still looking into this?
cheers

Addressed requests

LGTM, as far @rjmccall 's concern about documentation is addressed.

In D75169#1952159, @pratlucas wrote:

Why not just make half as an argument do the right thing for that case?

That would be the ideal approach, but currently there's a limitation on the backend's calling convention lowering that gets in the way.
The lowering of calls in SelectionDAGBuilder includes a target-independent step that is responsible for spliting or promoting each argument into "legal registers" and takes place before the targets' calling convention lowering.
As f16 is not a legal type on many of the AAPCS_VFP targets, it gets promoted to f32 before the target's lowering code has a chance to define how to handle it.
Ideally, this stpe should only take place if lowering calling conventions after type legalization - there's a FIXME there already capturing that -, but that would involve a major rewriting that would impact multiple targets.
Inserting a hacky target-dependent fix in this step also didn't look very good.
Do you see other alternatives for handling it? If not, which approach would you suggest?

Would it be possible to pass a half argument and fix-it-up at CodeGenPrepare?

From my point it does LGTM.

Ping

I believe we can avoid creating some blocks for latter removing them, no?

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.

Hi @davide, do you have any plans to address this issue? Regards.

Now testing only simple-register-coalescing

Fixed x86 test

Do not perform register coalescing

Hi @efriedma and @rnk ,

Ping. I would like to move along this small patch.

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

LGTM, not forgetting to remove the exit comments.

Hi @efriedma,
thanks for looking into this.

Ping

Ping

Gentle Ping. I believe all issues with this fix have been fulfilled.

• Fixed tests to use "\\b" instead of "\b".

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.

Ping.

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

• 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:

• Correct mayAlias to isStack

• Fixed tests
• Avoid optimization when target is Windows with CFI, as we need to correctly update unwind information

Ping.

LGTM, after a nit inline.

Do not optimize post-increment SP in windows targets

In D75755#1910057, @efriedma wrote:

I think there's a secondary problem here: if we move an SP adjustment, we might also need to adjust the unwind/debug info. This is particularly nasty on Windows, where messing up the unwind info can actually cause a miscompile.

If you don't want to spend the time to try to figure that out, I'd be happy to just completely forbid optimizing sp adjustments here. It should be rare enough that it doesn't matter much in practice.

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>;
}

Changes requested: IR test, chain changed depending on isStrictFPOpcode, signed and unsigned tests

Hi @ostannard, thanks for your review.
I updated the patch so it won't act when the computed volatile bit-field access will overlap a zero length bit-field, avoiding the conflict. We can update it accordingly to future versions of the AAPCS if required.

• Fixing comments and removing some tests changes not required

• Add test that the volatile access does not cross a border defined by a zero lenght bit-field, defined by C11, avoiding race-conditions

In D73970#1865039, @smeenai wrote:

Do you think it's worth cherry-picking this to 10.0? CC @hans

Also, it looks like the test got dropped in the commit.