- User Since
- Jan 20 2014, 9:18 PM (290 w, 5 d)
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.
May 6 2016
Apr 13 2016
Mar 18 2016
Yes, in its current usage this warning is only used for Apple's SDKs, but how does it help to put "apple" in the name of the diagnostic? Are you concerned about a name conflict with a similar diagnostic for non-Apple SDKs?
Mar 15 2016
This is a good idea overall. See comments.
Mar 11 2016
Thanks Ben. Committed in r263299
Mar 9 2016
I do like the explicit nature of this approach, but I'm worried about it being too novel. For instance, would this compel GCC to implement a similar parsing feature since they also support the deprecated attribute, that sort of thing. Also, it feels like a bit of an odd design in C and C++ since nothing else in the language supports argument passing by name like that (and I would really hate to see us use = blah only to find out that C++ someday standardizes on something different).
Mar 8 2016
Mar 7 2016
Mar 3 2016
The object files have a new SF_AltEntry flag to distinguish symbols that do not start a new atom. ld64 has had support for that for a while now.
Feb 12 2016
Committed in r260787.
Feb 11 2016
Jan 5 2016
I applied a Darwin-specific change for this to clang in r256026.
This looks good to me. Thanks for working on this!
Dec 16 2015
There has been a long-standing convention in both gcc and clang for Darwin that -static changes the default for PIC. Changing that convention is breaking stuff for us. The commit message for r245667 says "This new behavior also matches GCC", but that is not correct, at least for Darwin. I just tried compiling a simple test file with gcc-4.8.2 for OS X. If you run "gcc -v" with and without -static, you'll see that the gcc driver adds -fPIC to the cc1 command only when you do not use -static. I only have access to the GCC sources from an old version, but the config/i386/darwin.h file has this:
Dec 14 2015
Regarding the FIXME in lib/Frontend/CompilerInvocation.cpp: I agree with Hal that you can remove that. We used to complain about unsupported -fno-builtin-* options (and until now they have *all* been unsupported), but in r191434, Rafael changed clang to silently ignore those options, with the explanation that it matches gcc's behavior. Assuming that gcc has not changed in that regard, it makes sense that we should continue to ignore -fno-builtin-* for unsupported options.
Oct 9 2015
Alex, you guys were right. I hadn't thought of all the cases we would need to check for. I agree that we should have a simple mechanism to distinguish content from different vendors.
Sep 23 2015
Good idea. The patch looks good. It will need to be coordinated with a change to compiler_rt build cc_kext_ios.a, though. Thanks for cleaning this up.
Sep 8 2015
On Darwin platforms, the libc++ headers are expected to be installed alongside clang. If you're not doing that, then you're building it wrong. Adding more fallback options for finding the headers just makes things worse, because instead of a clear failure, you're more likely to get something that works but behaves badly in a subtle way (e.g., due to using really old libc++ headers).
Jan 16 2015
Dec 22 2014
I'm not aware of anything using this. Adding Nick in case he knows of anything.
Nov 3 2014
As far as I know, it should be fine to remove cortex-a9-mp. See my reply from last January:
Aug 7 2014
This is OK to commit. The option description is currently:
Aug 4 2014
This is good to commit, but see my suggested improvement below. I assume it will be tested once we get the llvm-cov support enabled?
Jul 25 2014
Hard to argue with that one! Please go ahead and commit.
Jul 24 2014
Looks good. Please commit!
Jul 16 2014
The documentation is going to need more work. Let’s split that out into a separate patch. At least it helps me understand how the reader and writer work.