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.

Added opt-out flag

Revordered some tests adding and definition of the "isAAPCS" function from patch D72932

Updated wrong patch here.

• Removed test
• Added clear at the end of run as well, to clear waste
• Moved clearing to a more sensible position

• Removed test
• Added clear at the end of run as well, to clear waste
• Moved clearing to a more sensible position

Ping :-)

In D73970#1857182, @samparker wrote:

Does the test work without debug info? Would be good to cut down the size of it.

• Added flag to allow user to opt-in

• Do not generate special volatile access if the record alignment is smaller than the bit-field declared type

• Moved computation of volatile accesses to the record layout builder

In D72932#1829716, @ostannard wrote:

Why are you doing this in CodeGen, rather than adjusting the existing layout code in CGRecordLowering? Doing it this way will result in AdjustAAPCSBitfieldLValue being called for every access to the bitfield, rather than just once. This is probably more fragile too, because it's spreading the logic across multiple parts of the codebase, and has to undo some of the layout done by CGRecordLowering.

Sorry, submitted using ide by mistake. Already reverted it.

In D72932#1829716, @ostannard wrote:

Why are you doing this in CodeGen, rather than adjusting the existing layout code in CGRecordLowering? Doing it this way will result in AdjustAAPCSBitfieldLValue being called for every access to the bit-field, rather than just once. This is probably more fragile too, because it's spreading the logic across multiple parts of the codebase, and has to undo some of the layout done by CGRecordLowering.

• Fixed to return correct token

Created a early exit when a source location overflow is detected,
avoiding further processing or reaching other assert failures.
For that, it was required to create a 'Failure' kind for includes,
as well add return points along the callstack.

• Fixed the destination register class when converting ADDrr to ADDri or ADDspImm. Added test
• Fixed the number of arguments when converting t2SUBspImm to tSUBspi by adding a register at the end. Else, it would break llvm-mca that expects the number of operands to follow the table-gen, even when not used by the instruction.

In D70680#1813841, @dnsampaio wrote:

Looking into CodeGen/Thumb2/thumb2-mov.ll test failure.

Looking into CodeGen/Thumb2/thumb2-mov.ll test failure.

Requested fixes

Requested fixes:

Removed dead/wrong tablegen t2InstSubst, defining a t2ADDri12 using t2_so_imm_neg instead of imm0_4095_neg

• undo some clang-format-diff changes

Added fixme to ARMLoadStoreOptimizer due negative offset

Added test-cases for all added (and fixed existing) thumb2 add/sub alias

Added test