Page MenuHomePhabricator

bob.wilson (Bob Wilson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 20 2014, 9:18 PM (256 w, 1 d)

Recent Activity

Fri, Dec 7

bob.wilson accepted D55442: Update the Swift version numbers reported by objdump.

LGTM, too.

Fri, Dec 7, 10:13 AM

May 25 2018

bob.wilson closed D46550: Support Swift calling convention for PPC64 targets.

Committed in Clang r333316

May 25 2018, 2:30 PM

May 7 2018

bob.wilson added a comment to D46550: Support Swift calling convention for PPC64 targets.

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

May 7 2018, 2:40 PM
bob.wilson created D46550: Support Swift calling convention for PPC64 targets.
May 7 2018, 2:37 PM

Mar 21 2018

bob.wilson added a comment to D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros.

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.

Mar 21 2018, 1:31 PM

Jan 3 2018

bob.wilson added a comment to D41672: support phi ranges for machine-level IR.

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.

Jan 3 2018, 9:10 PM
bob.wilson closed D41672: support phi ranges for machine-level IR.

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

Jan 3 2018, 6:59 PM

Jan 2 2018

bob.wilson abandoned D19080: allow SSAUpdater to be used for Swift.

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.

Jan 2 2018, 11:18 AM
bob.wilson created D41672: support phi ranges for machine-level IR.
Jan 2 2018, 11:16 AM
bob.wilson closed D36563: Add a getName accessor for ModuleMacros.
Jan 2 2018, 11:04 AM

Dec 19 2017

bob.wilson accepted D41425: [darwin][driver] Warn about mismatching -<os>-version-min rather than superfluous -<os>-version-min compiler option.

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 19 2017, 6:06 PM

Dec 15 2017

bob.wilson accepted D40998: [driver][darwin] Take the OS version specified in "-target" as the target OS instead of inferring it from SDK / environment.

LGTM

Dec 15 2017, 9:46 PM
bob.wilson accepted D41076: [driver][darwin] Set the 'simulator' environment when it's specified in '-target'.

LGTM

Dec 15 2017, 8:50 PM
bob.wilson added a comment to D41087: [Preprocessor] Implement __is_target_{arch|vendor|os|environment} function-like builtin macros.

Thanks! LGTM

Dec 15 2017, 4:30 PM

Dec 14 2017

bob.wilson added a comment to D41087: [Preprocessor] Implement __is_target_{arch|vendor|os|environment} function-like builtin macros.

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 14 2017, 11:04 PM

Dec 9 2017

bob.wilson requested changes to D40998: [driver][darwin] Take the OS version specified in "-target" as the target OS instead of inferring it from SDK / environment.
Dec 9 2017, 9:26 PM

Dec 4 2017

bob.wilson accepted D40682: [driver] Set the 'simulator' environment for Darwin when -m<os>simulator-version-min is used.
Dec 4 2017, 6:04 PM

Nov 30 2017

bob.wilson added inline comments to D40682: [driver] Set the 'simulator' environment for Darwin when -m<os>simulator-version-min is used.
Nov 30 2017, 9:14 PM
bob.wilson requested changes to D40682: [driver] Set the 'simulator' environment for Darwin when -m<os>simulator-version-min is used.
Nov 30 2017, 5:06 PM

Oct 31 2017

bob.wilson added a comment to D39446: [PGO] Detect more structural changes with the stable hash.

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 31 2017, 9:47 AM

Oct 23 2017

bob.wilson closed D39143: Add a new Simulator entry for the target triple environment.

Thanks, Eric!
r316380

Oct 23 2017, 2:53 PM

Oct 20 2017

bob.wilson created D39143: Add a new Simulator entry for the target triple environment.
Oct 20 2017, 1:51 PM

Aug 10 2017

bob.wilson added a comment to D36563: Add a getName accessor for ModuleMacros.

Committed in r310622

Aug 10 2017, 9:44 AM

Aug 9 2017

bob.wilson created D36563: Add a getName accessor for ModuleMacros.
Aug 9 2017, 5:21 PM

Jun 30 2017

bob.wilson accepted D34529: [Driver] Check that the iOS deployment target is iOS 10 or earlier if the target is 32-bit.

LGTM

Jun 30 2017, 4:45 PM
bob.wilson added inline comments to D34529: [Driver] Check that the iOS deployment target is iOS 10 or earlier if the target is 32-bit.
Jun 30 2017, 1:09 PM

Jun 29 2017

bob.wilson added a comment to D34529: [Driver] Check that the iOS deployment target is iOS 10 or earlier if the target is 32-bit.

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?

Jun 29 2017, 10:25 PM

Mar 6 2017

bob.wilson closed D30578: remove Cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING.

All the more reason to clean this up now!
r297109

Mar 6 2017, 5:03 PM

Mar 3 2017

bob.wilson created D30578: remove Cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING.
Mar 3 2017, 8:32 AM

Feb 14 2017

bob.wilson closed D29919: allow migrating away from cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING.

Committed in r295090

Feb 14 2017, 11:18 AM

Feb 13 2017

bob.wilson created D29919: allow migrating away from cmake option for LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING.
Feb 13 2017, 5:39 PM

Jan 30 2017

bob.wilson added a comment to D28934: Write a new SSAUpdater.

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 30 2017, 4:03 PM

Jan 5 2017

bob.wilson closed D28265: disable sigaltstack on Apple platforms.

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

Jan 5 2017, 6:38 PM

Jan 4 2017

bob.wilson added a comment to D28265: disable sigaltstack on Apple platforms.

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 4 2017, 10:21 AM

Jan 3 2017

bob.wilson retitled D28265: disable sigaltstack on Apple platforms from to disable sigaltstack on Apple platforms.
Jan 3 2017, 4:45 PM

Dec 5 2016

bob.wilson added a comment to D27432: Introduces cmake option `LLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING`.

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

Dec 5 2016, 4:47 PM

May 6 2016

bob.wilson updated subscribers of D19403: Add loop pragma for Loop Distribution.
May 6 2016, 5:24 PM

Apr 13 2016

bob.wilson retitled D19080: allow SSAUpdater to be used for Swift from to allow SSAUpdater to be used for Swift.
Apr 13 2016, 3:41 PM

Mar 18 2016

bob.wilson added a comment to D18088: Add a new warning to notify users of mismatched SDK and deployment target.

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 18 2016, 4:56 PM

Mar 15 2016

bob.wilson added a comment to D18088: Add a new warning to notify users of mismatched SDK and deployment target.

This is a good idea overall. See comments.

Mar 15 2016, 4:16 PM

Mar 11 2016

bob.wilson closed D17941: add fix-its for format-security warnings.
Mar 11 2016, 2:01 PM
bob.wilson accepted D17941: add fix-its for format-security warnings.

Thanks Ben. Committed in r263299

Mar 11 2016, 2:00 PM

Mar 9 2016

bob.wilson added a comment to D17865: Add an optional string argument to DeprecatedAttr for Fix-It..

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 9 2016, 12:49 PM
bob.wilson updated subscribers of D17865: Add an optional string argument to DeprecatedAttr for Fix-It..
Mar 9 2016, 12:11 PM

Mar 8 2016

bob.wilson added a comment to D17941: add fix-its for format-security warnings.

What about wprintf? Do we currently warn for wprintf(str)? If so, then the fixit probably needs to involve L"%ls".

Mar 8 2016, 9:10 AM

Mar 7 2016

bob.wilson retitled D17941: add fix-its for format-security warnings from to add fix-its for format-security warnings.
Mar 7 2016, 2:24 PM

Mar 3 2016

bob.wilson added a comment to D17854: add .alt_entry assembly directive for Mach-O.

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.

Mar 3 2016, 12:20 PM
bob.wilson retitled D17854: add .alt_entry assembly directive for Mach-O from to add .alt_entry assembly directive for Mach-O.
Mar 3 2016, 9:38 AM

Feb 12 2016

bob.wilson closed D17166: [Sema] More changes to fix Objective-C fallout from r249995..

Committed in r260787.

Feb 12 2016, 5:50 PM

Feb 11 2016

bob.wilson retitled D17166: [Sema] More changes to fix Objective-C fallout from r249995. from to [Sema] More changes to fix Objective-C fallout from r249995..
Feb 11 2016, 1:12 PM

Jan 5 2016

bob.wilson closed D15455: [Driver] Let -static override the toolchain default PIC setting..
Jan 5 2016, 11:30 AM
bob.wilson accepted D15455: [Driver] Let -static override the toolchain default PIC setting..

I applied a Darwin-specific change for this to clang in r256026.

Jan 5 2016, 11:29 AM
bob.wilson accepted D15195: PR4941: Add support for -fno-builtin-foo options..

This looks good to me. Thanks for working on this!

Jan 5 2016, 9:03 AM

Dec 16 2015

bob.wilson added a comment to D15455: [Driver] Let -static override the toolchain default PIC setting..

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 16 2015, 3:59 PM

Dec 14 2015

bob.wilson added a comment to D15195: PR4941: Add support for -fno-builtin-foo options..

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.

Dec 14 2015, 5:35 PM

Oct 9 2015

bob.wilson added a comment to D12618: Reserve a vendor reserved block ID for bitcode.

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.

Oct 9 2015, 10:12 AM

Sep 23 2015

bob.wilson accepted D13112: [darwin] [builtins] Stop generating cc_kext_ios5 and move iOS architectures out of cc_kext into cc_kext_ios.

Looks good.

Sep 23 2015, 3:07 PM
bob.wilson added inline comments to D13112: [darwin] [builtins] Stop generating cc_kext_ios5 and move iOS architectures out of cc_kext into cc_kext_ios.
Sep 23 2015, 1:29 PM
bob.wilson added inline comments to D13112: [darwin] [builtins] Stop generating cc_kext_ios5 and move iOS architectures out of cc_kext into cc_kext_ios.
Sep 23 2015, 12:51 PM
bob.wilson accepted D13113: [darwin] [builtins] Stop generating cc_kext_ios5 and move iOS architectures out of cc_kext into cc_kext_ios.

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 23 2015, 11:51 AM

Sep 8 2015

bob.wilson added a comment to D12646: Add libc++ header path for DarwinClang builds.

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).

Sep 8 2015, 9:21 AM

Jan 16 2015

bob.wilson added a comment to D6870: Adding option -fno-inline-asm to disallow inline asm.
In D6870#109832, @rnk wrote:
  • Update patch that AsmLabels are now disabled with -fno-gnu-inline-asm. This also means functions with asm-labels will also be reported as an error with -fno-gnu-inline-asm.

I wasn't suggesting that we reject function asm labels, I was suggesting that we reject variables marked as living in specific registers. Consider something like:

register void *sp asm("esp");
int main () {
  printf("sp: %p\n", sp);
}

This code compiles down to read the esp register in main.

I think using asm labels to rename something is not inherently architecture specific and could be accepted even in the presence of -fno-inline-asm. What do you think?

Jan 16 2015, 11:12 AM

Dec 22 2014

bob.wilson added a reviewer for D6574: Remove darwin_fat.mk.: kledzik.

I'm not aware of anything using this. Adding Nick in case he knows of anything.

Dec 22 2014, 8:26 PM

Nov 3 2014

bob.wilson added a comment to D6057: Remove the cortex-a9-mp CPU..

As far as I know, it should be fine to remove cortex-a9-mp. See my reply from last January:

Nov 3 2014, 8:23 AM

Aug 7 2014

bob.wilson added a comment to D4799: Add a cc1 option called dump-coverage-mapping.

This is OK to commit. The option description is currently:

Aug 7 2014, 4:01 PM

Aug 4 2014

bob.wilson added a comment to D4746: Coverage: add HasCodeBefore flag to a mapping region.

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?

Aug 4 2014, 10:32 AM

Jul 25 2014

bob.wilson added a comment to D4677: Coverage: remove empty mapping regions.

Hard to argue with that one! Please go ahead and commit.

Jul 25 2014, 3:21 PM · deleted

Jul 24 2014

bob.wilson added a comment to D4301: Initial code coverage mapping data, reader, writer + C interface for ProfileData.

Looks good. Please commit!

Jul 24 2014, 4:17 PM · deleted

Jul 16 2014

bob.wilson added a comment to D4301: Initial code coverage mapping data, reader, writer + C interface for ProfileData.

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.

Jul 16 2014, 10:38 PM · deleted