OK, that doesn't sound like it will be a problem

There's a typo in the first new paragraph: "adresses"

LGTM, too.

Committed in Clang r333316

Previous review (for the swift-llvm GitHub repo): https://github.com/apple/swift-clang/pull/167

Sorry that I missed your earlier comment about this. The confusion could only arise in the context of a tool (like a compiler) that is being used for cross-compilation. That is a small fraction of the audience for Clang, and we should design this in a way that makes the most sense for the majority of users. If there's a naming scheme that is better for both, then we should do that, but I don't think this is it.

Unfortunately that unit test doesn't work. I didn't notice the problem but the sanitizer bot exposed it. The existing bogus target in MachineInstrTest.cpp is only good enough to create instructions but not sufficient to insert them into basic blocks. The addNodeToList ilist callback dereferences the pointer to the MachineRegisterInfo. Adding MachineRegisterInfo would also require TargetRegisterInfo, and even a minimal implementation of that would be quite complicated. If anyone has a good idea of an alternative, please let me know. In the meantime, I have removed the unit test in r321784.

OK. Committed with Davide's suggested changes in r321783.

Chandler came up with a much better solution for phi iterators in r303964.
I have proposed a similar change for machine-level IR in https://reviews.llvm.org/D41672
Abandoning this one.

Eventually it would be nice to also warn about redundant -m*-version-min options, but for now I agree that it would be best to start with warning only when the options are different.

LGTM

LGTM

Thanks! LGTM

I'm concerned here about the check for the names as written vs. the canonical names. @compnerd pointed out one specific case with armv7, and I see that you've got special handling for "darwin", but I think there are more. What about "arm64" vs. the canonical "aarch64"? Look through the triple parsing code in Triple.cpp and I'm pretty sure you'll find more. I had been thinking about using the canonical names. However, that's not ideal either because the canonical names intentionally exclude suffixes that some users may want to distinguish (e.g., armv7 vs armv7k).

I'm excited to see some progress on this, but since there is overhead to adding a new hashing scheme, I think we should do more before introducing a new scheme. One of the problems with the previous scheme is that is did not take into account nesting. Distinguishing an if-statement from an if-else statement is good but we also need to distinguish what code is inside the then-block, else-block, and the code afterward. As it stands, I believe the following are all hashed the same, even with this patch:

Thanks, Eric!
r316380

Committed in r310622

LGTM

The proposed error message does not provide any information about why the version is invalid. That could be confusing. Your comment in the code is more clear: "iOS 10 is the maximum deployment target for 32-bit targets". Can you say something like that in the error message?

All the more reason to clean this up now!
r297109

Committed in r295090

It's great to see some new work in this area. It has been years since I rewrote the SSAUpdater and I've always been worried about the performance. It seems fast enough for the common cases but the worst-case behavior is terrible. Anyway, it has been a long time since I looked at this, but I remember concluding that it was hard to really fix it right without changing the API. The basic part of the Braun et al. approach looks very similar to what we had prior to my rewrite (r100047 through r101612). Unfortunately it has been a long time and I'm having trouble remembering the details of the problem we ran into. The gist of it was that SSA construction is different than SSA updating because the latter needs to determine if the necessary phis are already present. We ran into cases where the SSA updater was unable to determine that a loop already had a suitable phi, so it would add a new one, creating phi cycles that grew by one every time the SSA updater was invoked on the code (which can be a large number of times). Perhaps this only came up with irreducible control flow or something, but basically you've got to get minimal SSA all the time to avoid this situation.

Committed in r291206, with an expanded comment.
I also reverted my previous change to use _Unwind_Backtrace (r291207).

Yes, of course you are right that this is not ideal. It's not about avoiding crashes though -- the issue is that the backtraces we dump out after an assertion failure are useless since they stop at the signal handler.

This looks good to me. I tried it out and it works as expected.