Address feedback

In D146536#4210062, @sgraenitz wrote:

Thanks for tackling this! Yes, I hit it as well during development and wondered what is the right thing to do. The effect of the proposed change will be log messages like this right?

... which resolves to a constant value of mismatched width (-1 bits). Ignoring constant.

Update to include additional behaviour and reference INCLUDE and LIB.

Looks reasonable I guess - but I think it would be good to mention the env variables INCLUDE and LIB too, for alternative ways of finding the same things - even if it's not strictly the same as what this new section talks about.

Yeah, I think that we will have to slowly erode the differences, the model for the pair-wise relocations here is a challenge.

Sure thing, I can try to write up some details about that @hans!

Just a friendly reminder, I'd like to get this merged to do a follow up change to improve some of the usability of these flags.

Yeah, I suspect that would be difficult. Additionally, these are aliases, so they should already have testing coverage through the existing tests.

We do have other examples of ternary state options., and making this ternary for the sake of compatibility is an intriguing idea. However, I also ascribe to POLA/POLS. Mapping LLVM_ENABLE_UNWIND_TABLES to -funwind-tables seems pretty reasonable to me. The only time that you would want to disable these is if it is an embedded context where you do not care about debugging or crash analytics, where you should pretty quickly see the size impact. If this plays out to be a problem to be enabled by default we can always revisit the ternary state.

I think that the current release note should be sufficient. This is handling the same scenario in a different path.

Use getSignificantBits

@thakis and https://github.com/llvm/llvm-project/issues/59640

In D140347#4009008, @tschuett wrote:

There is as trySextValue in APInt: https://reviews.llvm.org/D139683. Maybe you need a tryMul.

Further reduce the test case. We create the splat and shuffle, offset it by vlenb and return the generated splat. This reduces down to the minimal assembly sequence that would exhibit the issue. If we were to accidentally truncate the scale, we would drop the read of vlenb and the adjustment for the shuffle.

Further reduce test case by creating a hand written synthetic test case. We can get away with a 2-iteration unrolled loop avoiding any phi branches. The previous overflow would improperly remove the increment in between the two iterations.

Partially address feedback from Craig, address feedback from Jessica.

In D140347#4006583, @craig.topper wrote:

BTW, why is this in SimplifyDemandedBits instead of computeKnownBits? It doesn't use the DemandedBits.

Given the UB and potential miscompile, reverted in 9b92f70d4758f75903ce93feaba5098130820d40.

I'm seeing a potential miscompilation on RISCV64 with this particular change: https://github.com/llvm/llvm-project/issues/59594

Thanks @fsb4000 (and @CaseyCarter)! I think that due to the shipped version, it makes sense to do this still. Using standard semantics in general I think is preferable, since nothing prevents another compiler implementation to still do something similar.

Seems that this alone is insufficient as some build does run into issues, so I may need to refine this further.

@fsb4000 is my reading correct that MSVC will look into trying to handle static inline even though it is a GNUism? I wonder if we should consider limiting the use of static inline to C mode rather than including C++. I also wonder if I can loosen it similarly to avoid the issue I'm running into (which @STL_MSFT correctly identified - modules).

Thanks! This looks good. Personally, I'd use 0x8 instead of 8, which might make it more obvious that this is intentional (and that it is a bitmask).

Could you please also add a test case for .rodata.suffix as that is added as a case as well.

In D139585#3985179, @benwolsieffer wrote:

No, __ARM_ARCH_6__ is only defined on base ARMv6. __ARM_ARCH_6K__ is defined instead on ARMv6K. I can't find this clearly documented anywhere, but I have confirmed this behavior through experimentation with GCC and by examination with clang (see implementation here)

On the other hand, there is the __ARM_FEATURE_LDREX macro, which provides the exact functionality we want here. The only problem is that it is deprecated in the ARM C Language Extensions specification, although it is still defined even in the latest versions of the ACLE. This macro was not used in https://reviews.llvm.org/D5490 in part because it was deprecated, but mostly because clang did not implement it at the time.

What do you think about using that macro instead, despite the deprecation?

In D139585#3982211, @benwolsieffer wrote:

That's not quite right though, since that excludes ARMv6K and ARMv6Z (which is a superset of ARMv6K). The only ARMv6 variant that doesn't support these instructions is base ARMv6 (and technically ARMv6J, but LLVM doesn't appear to consider that as a separate target). __ARM_ARCH_6__ is defined only when targeting base ARMv6, not any of the other variants.

According to https://reviews.llvm.org/D95891, these instruction are not supported in ARMv8-M either, so we don't want to use __ARM_ARCH > 7.

(Accepting Revision for Closing)
I missed the trailing number of the differential revision, and it failed to tie it to this. The commit is at https://github.com/llvm/llvm-project/commit/707cc06e1570b5966efcd6a9124191c80fa7a754
@rnk is going to be out for a while, so any concerns can be addressed in a follow up.

Might be useful to add a reference to https://developer.arm.com/documentation/dht0008/a/ch01s02s01 in the commit message.

Seems like a straightforward change for the tests to repair them on Android.

This is definitely a nice cleanup. There is a way to use .irp to have it count, but this is fine for now.

Update to avoid use of the -0 trick.

Add a test case for member data.

Address feedback from review