- User Since
- Jan 20 2014, 9:18 PM (371 w, 1 d)
Dec 12 2019
Oct 29 2019
Mar 7 2019
OK, that doesn't sound like it will be a problem
Jan 28 2019
There's a typo in the first new paragraph: "adresses"
Dec 7 2018
May 25 2018
Committed in Clang r333316
May 7 2018
Previous review (for the swift-llvm GitHub repo): https://github.com/apple/swift-clang/pull/167
Mar 21 2018
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.
Jan 3 2018
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.
Jan 2 2018
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.
Dec 19 2017
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.
Dec 15 2017
Dec 14 2017
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).
Dec 9 2017
Dec 4 2017
Nov 30 2017
Oct 31 2017
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:
Oct 23 2017
Oct 20 2017
Aug 10 2017
Committed in r310622
Aug 9 2017
Jun 30 2017
Jun 29 2017
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?
Mar 6 2017
All the more reason to clean this up now!
Mar 3 2017
Feb 14 2017
Committed in r295090
Feb 13 2017
Jan 30 2017
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.
Jan 5 2017
Committed in r291206, with an expanded comment.
I also reverted my previous change to use _Unwind_Backtrace (r291207).
Jan 4 2017
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.
Jan 3 2017
Dec 5 2016
This looks good to me. I tried it out and it works as expected.