Will be continued as a GitHub PR

In D156396#4539673, @efriedma wrote:

I think with fb7c3807, we shouldn't use PRE_DEC/POST_DEC modes anymore. So IsDec will always be false, and this patch does nothing.

Here's a bit more context:

Remove to_string usages

Thank you for reviewing, and yes, I totally aggree. Preventing such issues from happening in the first place is would be better.
Will update this change tomorrow to drop the to_string calls.

Is there anything missing for this to be committed?

I created a fix: https://reviews.llvm.org/D155093

I thought about static_cast'ing Val instead of changing the format string. In case the underlying type of Val changes in the future, one would otherwise have to adapt the format string as well.
Or is there a way to emit a compiler warning if the format string mismatches? You mentioned marking llvm::format with a format attribute, @eli.friedman? Does that trigger a warning?

In D153800#4490748, @efriedma wrote:

snprintf(buf, bufsize, "%d", 1LL) has undefined behavior.

In D153800#4490477, @efriedma wrote:

It seems that the calling convention snprintf expects is all values in registers, so r0, r1, r2, r3. Meaning it expects the Value to be in r3.

A "long long" can't fit into r3.

The normal calling convention rules (https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#parameter-passing) require skipping r3 and storing the "long long" value on the stack. So snprintf should ignore the value in r3 unless we're passing in a bad format string. (Which is possible, I guess... it looks like the "llvm::format" function isn't marked with a format attribute, so the format string isn't checked anywhere.)

I think I figured it out:

Here is the IR code of that specific function. The first load has an align of 4, which used to be excluded by the load optimization before this commit.

I managed to reproduce this and isolate the function responsible for the triggering the bug. It's llvm::format_object<long long>::snprint(char*, unsigned int) const. I haven't dug deeper yet, but that's at least some progress :D

In D154575#4476804, @RKSimon wrote:

cheers - lgtm

@DavidSpickett do you have instructions for me on how to setup everything to reproduce the problem.
So I can support you with the investigations.

In D153800#4468445, @DavidSpickett wrote:

I've reverted this due to a failure on all of Linaro's 2 stage 32 bit Arm bots, both v7 and v8 (though both run on v8 hardware).

For example:
https://lab.llvm.org/buildbot/#/builders/178/builds/5128
https://lab.llvm.org/buildbot/#/builders/187/builds/11994
https://lab.llvm.org/buildbot/#/builders/182/builds/6724

            7: error: Simplified template DW_AT_name could not be reconstituted: 
check:10'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            8:  original: f3<unsigned char, (unsigned char)'\x00'> 
check:10'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            9:  reconstituted: f3<unsigned char, (unsigned char)'\x7f'>

I haven't dug deeper yet but if that is one byte misplaced, it could make some sense.

Do you have access to a machine where you can reproduce this? A 32 bit container running on 64 bit hardware will also work.

If you do not, I can do the investigation.

In D153800#4450112, @efriedma wrote:

LGTM

I suspect we're actually missing a check somewhere for LDRD formation post-RA. But this looks like an improvement.

(I'll commit for you.)

as soon as this is commited,I will try to adjust the ARMLoadStoreOptimizer accordingly. @eli.friedman you have commit permissions, right? would you mind committing this change for me with the name and email provided above?

In D152790#4438421, @efriedma wrote:

LGTM

(I'm guessing you don't have commit access? Please let me know the author name/email you want on the commit.)

Thank you for the hint. Using update_llc_test_checks.py makes writing the tests much easier.

Added an armv7 strict case to make sure it is identical to non-strict for align 4

I added an align 4 test for armv7 and armv6 with and without strict alignment.
I omitted armv5 here, as, due to the missing strict align, the test generates an ldrd which is probably not something one wants to explicitly test for.
Question is, should I add strict align to armv4/v5 test cases here and adjust the test accordingly?

Thank you for investigating it. I have added a helper called getDualLoadStoreAlignment. Feel free to suggest a better name if you have one.

In D152790#4419917, @efriedma wrote:

I have no idea why the ARMLoadStoreOptimizer code was written that way; maybe fix that too, for the sake of clarity.

I did have Align(4) initially, but saw that it was done differently here: https://github.com/llvm/llvm-project/blob/2a1716dec57e8b3dd668df17ecbedfc77a4112e5/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp#L2294
I simply copied the expression. If that is not applicable here, I can just use 4 instead or respect allowsUnalignedMem.

Seems like this can be merged, right?

In D115456#3248313, @hans wrote:

Looks great, thanks! Maurice, do you want to add a note about this to docs/ReleaseNotes.rst?

In D115456#3217857, @majnemer wrote:

In D115456#3217595, @momo5502 wrote:

In D115456#3216811, @majnemer wrote:

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

The MS compiler only emits the dynamic initializers for variables with constructors/destructors, just like it is currently done here for the Itanium ABI.
I also thought about adopting that behaviour, but I think threre are edge-cases when triggering dynamic TLS initialization even for constant variables is useful.
For example there might be custom TLS callbacks that can affect the value of this variable.

If desired, I can change it to match the behaviour of MS, but I thought it could be beneficial to diverge in this case.

IMO, it is probably best to match behavior here within reason (ignoring bugs latent in MSVC) here. My thinking is that in the face of COMDATs, we cannot ensure which copy of an inline function will make its way to the binary and different link orders would provide different behavior.

In D115456#3216811, @majnemer wrote:

This is looking great! Just a few more questions.

What is the behavior with something like:

thread_local int x = 2;
int f() {
  return x;
}

I'm wondering if we need to move this logic into the generic C++ ABI implementation.

Comments were addressed

A call to isCompatibleWithMSVC was added with the proper version that introduced the change (1925) and comments describing the intention of the change were added.