This appears to only happen with -fintegrated-cc1, with fno-integrated-cc1 it does not repro. Still looking into this.

Thanks for pinging on this @thegameg. I had been looking at this a day or two ago but got side-tracked. Will take another look.

In D71301#1818233, @thakis wrote:

This test fails on Windows: http://45.33.8.238/win/5658/step_7.txt

Looks like this expects itanium mangling; maybe you want -triple %itanium_abi_triple?

In D71301#1818571, @thakis wrote:

In D71301#1818233, @thakis wrote:

This test fails on Windows: http://45.33.8.238/win/5658/step_7.txt

Looks like this expects itanium mangling; maybe you want -triple %itanium_abi_triple?

Since the bot's been red for a while now, I went ahead and landed that fix in 84baf123. The bot's green now, but please check if that's what the test wants to test or if this just papers over things.

In D71301#1817926, @thakis wrote:

This breaks tests on macOS: http://45.33.8.238/mac/5672/step_7.txt (macOS prefixes functions with an extra _.)

In D71301#1817926, @thakis wrote:

This breaks tests on macOS: http://45.33.8.238/mac/5672/step_7.txt (macOS prefixes functions with an extra _.)

Going to do an NFC pre-commit before this to add the braces.

using isa

ping

Abandoning. Will post some piecemeal patches that handle the various MO types separately.

I'm going to create a new diff that is purely NFC, but moves this code to use the switch-statement. I will add multiple diffs them for specific things like CImm/FPImm. I'll eventually abandon this diff.

adding hash against RC/RB

Being more explicit about which MachineOperand types can be hashed and which ones can be skipped due to unstable hashing on pointers as @aditya_nandakumar mentioned. Also brought back hashing on a vreg's defining MI operand because that makes sense.

In D71301#1778834, @compnerd wrote:

Should probably add a check for __block variables.

Abandoning. Duplicate diff

Adding test case.

In D71182#1776159, @bogner wrote:

In D71182#1776145, @plotfi wrote:

In D71182#1775875, @compnerd wrote:

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

FWIW I'm not really convinced that std::transform is *ever* clearer or better any more since C++11 gave us auto and range-for.

In any case, the rest of this all seems like a nice clean up to me.

In D71182#1775875, @compnerd wrote:

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

Update to make llvm::transform more clear than standard for-loop

In D71182#1775771, @aditya_nandakumar wrote:

I don't know what the convention of LLVM is here - but I prefer the simple for loop with a push back vs a use of transform (which is not really transforming anything) which is confusing.

Updated based on @compnerd's feedback.

This commit is no good. I got some wrong inspiration from a lot of the code claiming random args in the driver. abandoning.

Updating comments and altering test as per @cishida and @compnerd's feedback

adding tests for weak/strong mismatch that is also a size/type mismatch for a given symbol.

Update based on @compnerd's feedback

In D70794#1765975, @aemerson wrote:

Could you upload a diff with more context (-U9999)?

Will land on Monday when holiday is over. I don't want to be that guy.

fixing indentation

Updated based on @compnerd's suggestions.

LGTM assuming @qcolombet or @aemerson have no objections.