dschuff (Derek Schuff)
Yet Another Compiler Compiler

Projects

User does not belong to any projects.

User Details

User Since
Jan 7 2013, 9:35 AM (254 w, 3 d)

Recent Activity

Thu, Nov 9

dschuff accepted D39873: [WebAssembly] Explicily disable comdat support for wasm output.

I'm fine with making this explicit for now. I'm not sure I'd yet go so far as to say that the goal is not to support comdats. We should definitely investigate how C++ and its debug info are supported in mach-o, but it may be that comdats are better than whatever they do. In general in the absence of good reasons (e.g. our 2-level namespace) I'd probably have a slight bias toward doing things the ELF way rather than the macho way; most people I've talked to who've worked a lot with macho don't speak too highly of it :)

Thu, Nov 9, 3:47 PM

Wed, Nov 8

dschuff accepted D39813: [WebAssembly] Update test expectations.
Wed, Nov 8, 11:58 AM

Tue, Nov 7

dschuff added a comment to D39748: [WebAssembly] Include GENERIC_TF_SOURCES.

Yes; in particular, musl's implementation of printf has some code where it promotes everything to long double, so if long double is f128, that causes several of these functions to get linked in to every binary, for no real benefit to the user.

Tue, Nov 7, 11:04 AM
dschuff added a comment to D39748: [WebAssembly] Include GENERIC_TF_SOURCES.

Yeah. Or not; I think you can still use __float128 types in clang, so if you did that then it would use these methods too. And as long as long double is f64 then none of these functions would get pulled into every program that uses printf (which is the case now), so it wouldn't impose any cost on anyone that doesn't actually use f128. So in that case there's not really a reason not to have it.

Tue, Nov 7, 11:00 AM
dschuff added a comment to D39748: [WebAssembly] Include GENERIC_TF_SOURCES.

Also btw I still think I want to back out the "long-double-is-f128" ABI from wasm. But either way it probably doesn't hurt to have these in compiler-rt.

Tue, Nov 7, 10:55 AM
dschuff accepted D39748: [WebAssembly] Include GENERIC_TF_SOURCES.

BTW I'm still trying to get caught up, so if there's some review or something you have in flight that you want me to get to first, just let me know.

Tue, Nov 7, 10:53 AM

Fri, Oct 27

dschuff accepted D39354: [WebAssembly] Add crt1.o when calling lld.
Fri, Oct 27, 10:57 AM

Oct 24 2017

dschuff accepted D39256: Add Triple::isOSUnknown.
Oct 24 2017, 3:10 PM
dschuff added a comment to D39256: Add Triple::isOSUnknown.

If you wanted to come up with your own set of syscalls or lower-layer stuff, You'd basically be making up your own OS, which you could name. But libclangrt.builtins in particular doesn't really depend on the OS, it's mostly just architecture-specific or processor-specific stuff. So you could reuse the clang support for that.

Oct 24 2017, 3:10 PM
dschuff added a comment to D39256: Add Triple::isOSUnknown.

D39218 uses the path we want want if the OS is 'unknown'. But shouldn't we make it so that any 'wasm-*-*' triple behaves the same way? i.e. someone who wants to put their own OS name in there but wants to use that same layout (or for that matter, maybe we want to adopt that in emscripten too).

Oct 24 2017, 2:25 PM
dschuff added a comment to D39256: Add Triple::isOSUnknown.

I don't necessarily object to this accessor.
But for our generic non-emscripten toolchain support that we are adding right now, maybe we should make it have whatever defaults we are building now with any OS (not just explicitly "unknown")?

Oct 24 2017, 1:46 PM
dschuff added a comment to D39250: Fix LLVM_LINK_LLVM_DYLIB=On build of llvm-cfi-verify.

oh wait it's in the subject line... just ignore me :D

Oct 24 2017, 12:29 PM
dschuff accepted D39250: Fix LLVM_LINK_LLVM_DYLIB=On build of llvm-cfi-verify.
Oct 24 2017, 12:28 PM
dschuff added a comment to D39250: Fix LLVM_LINK_LLVM_DYLIB=On build of llvm-cfi-verify.

Can you add to the commit description that this fixes the build with -DLLVM_LINK_LLVM_DYLIB=ON?

Oct 24 2017, 12:28 PM
dschuff accepted D39218: [WebAssembly] Include libclang_rt.builtins in the standard way.
Oct 24 2017, 10:25 AM

Oct 23 2017

dschuff added inline comments to D39218: [WebAssembly] Include libclang_rt.builtins in the standard way.
Oct 23 2017, 5:41 PM
dschuff accepted D39107: [WebAssembly] MC: Don't allow zero sized data segments.

Yeah, this seems OK for now.
I think at some point we may want to revisit the overall goals of the representation that we discussed originally (i.e. not having to encode a symbol table outside of wasm globals, a wasm object file possibly being directly loadable, etc), once we have a prototype and the constraints are clearer. Probably then it will be more clear what to do in cases like this.

Oct 23 2017, 4:23 PM

Oct 20 2017

dschuff added inline comments to D39107: [WebAssembly] MC: Don't allow zero sized data segments.
Oct 20 2017, 5:27 PM
dschuff accepted D39076: [WebAssembly] MC: Fix crash when -g specified.

LGTM, assuming I am correct in thinking that this will not affect the current wasm-elf assembly output (which does emit .loc directives).

Oct 20 2017, 1:37 PM
dschuff added a comment to D39107: [WebAssembly] MC: Don't allow zero sized data segments.

Should symbols actually be allowed to have the same value? How do we handle aliases?

Oct 20 2017, 1:20 PM

Oct 18 2017

dschuff accepted D39022: Don't set static-libs test feature when using LLVM_LINK_LLVM_DYLIB.
Oct 18 2017, 12:31 PM

Oct 10 2017

dschuff committed rL315335: [WebAssembly] Update MCObjectWriter and associated interfaces after r315327.
[WebAssembly] Update MCObjectWriter and associated interfaces after r315327
Oct 10 2017, 10:32 AM

Oct 9 2017

dschuff added a comment to D38640: [WebAssembly] Narrow the scope of WebAssemblyFixFunctionBitcasts.

Cool, and the code is even shorter this way too 👍

Oct 9 2017, 4:18 PM
dschuff accepted D38640: [WebAssembly] Narrow the scope of WebAssemblyFixFunctionBitcasts.
Oct 9 2017, 12:46 PM

Oct 6 2017

dschuff added inline comments to D38640: [WebAssembly] Narrow the scope of WebAssemblyFixFunctionBitcasts.
Oct 6 2017, 4:38 PM

Oct 5 2017

dschuff committed rL315022: [WebAssembly] Add the rest of the atomic loads.
[WebAssembly] Add the rest of the atomic loads
Oct 5 2017, 2:20 PM
dschuff closed D38523: [WebAssembly] Add the rest of the atomic loads by committing rL315022: [WebAssembly] Add the rest of the atomic loads.
Oct 5 2017, 2:20 PM
dschuff accepted D38597: [PEI] Remove required properties and use 'if' instead of std::function.
Oct 5 2017, 2:02 PM
dschuff added inline comments to D38597: [PEI] Remove required properties and use 'if' instead of std::function.
Oct 5 2017, 1:51 PM
dschuff updated subscribers of D38597: [PEI] Remove required properties and use 'if' instead of std::function.

Removing the requirement works fine now because the passes are set up correctly. The original intention behind getRequiredProperties in this case was that if the pass configuration were to be changed and PEI were to be called before regalloc on a target which wasn't prepared for that, there would be an easy assertion to say what went wrong.

Oct 5 2017, 1:29 PM

Oct 3 2017

dschuff updated the diff for D38523: [WebAssembly] Add the rest of the atomic loads.
  • remove spurious diff
Oct 3 2017, 5:40 PM
dschuff created D38523: [WebAssembly] Add the rest of the atomic loads.
Oct 3 2017, 5:33 PM

Sep 28 2017

dschuff accepted D38365: [WebAssembly] Revise the strategy for inline asm..

Oh, right, implicit locals. Yeah I don't think it's really worth thinking about too much.

Sep 28 2017, 9:41 AM
dschuff added a comment to D38365: [WebAssembly] Revise the strategy for inline asm..

This strategy seems fine to me.
How would this CL be different if it also affected the ELF flavor though? I don't mind fixing s2wasm if it makes LLVM simper.

Sep 28 2017, 9:19 AM

Sep 27 2017

dschuff accepted D38296: [WebAssembly] Allow each data segment to specify its own alignment.

Missed this yesterday, sorry.

Sep 27 2017, 1:24 PM

Sep 26 2017

dschuff accepted D38189: [WebAssembly] Use function/global index space in WasmSymbol.

I think it probably makes sense to use the function/global index space for the compiler and linker, and user-oriented tools (e.g. objdump, nm). For things like obj2yaml or readobj, maybe a something more raw would be appropriate, but I'd guess that it's not a super-high priority, other than it's nice to have those for testing.

Sep 26 2017, 3:30 PM
dschuff added a comment to D38189: [WebAssembly] Use function/global index space in WasmSymbol.

oh also it looks like you left a couple of words out of the first paragraph of the commit message.

Sep 26 2017, 3:30 PM
dschuff accepted D38246: [WebAssembly] Model weak aliases as wasm exports.

That can be a separate change though.
Is this in tool-conventions yet? If so, should update.

Sep 26 2017, 3:30 PM
dschuff accepted D37757: [WebAssembly] MC: Support for static init and fini sections.
Sep 26 2017, 3:30 PM
dschuff added inline comments to D38246: [WebAssembly] Model weak aliases as wasm exports.
Sep 26 2017, 3:30 PM

Sep 25 2017

dschuff added inline comments to D37606: [WebAssembly] Add Wasm exception handling prepare pass.
Sep 25 2017, 5:02 AM
dschuff added a comment to D37898: [TargetLowering] Correctly track NumFixedArgs field of CallLoweringInfo.

Thanks for the heads-up. WebAssembly's current varargs calling convention does pass the fixed args in one way (via declared function arguments) and the non-fixed args in another way (in memory), so in the cases where NumFixedArgs was wrong we would have been doing the wrong thing. However wasm also typechecks all calls against the declared function signature, so any wrong cases would fail in obvious ways (a signature mismatch error at validation time for direct calls, and at runtime for indirect calls). So just making the behavior correct should be the right thing to do. Also we have explicitly promised that we don't have a stable ABI yet, so that helps too :D

Sep 25 2017, 5:02 AM

Sep 20 2017

dschuff added a comment to D38111: [WebAssembly] Weak symbols should be defined in SF_Global.

retro-LGTM

Sep 20 2017, 4:55 PM
dschuff accepted D38096: [WebAssembly] Add support for local symbol bindings.
Sep 20 2017, 1:37 PM

Sep 19 2017

dschuff accepted D37886: [WebAssembly] Add support for naming wasm data segments.
Sep 19 2017, 1:10 PM

Sep 15 2017

dschuff added a comment to D37942: [WebAssembly] Restore __builtin_wasm_rethrow builtin.

(btw if there's a link in the commit message to the review, it should close automatically after commit)

Sep 15 2017, 10:26 PM
dschuff added a comment to D37931: Remove __builtin_wasm_rethrow builtin.

Right, I was actually referring to the VM's internal tracking of exception stacks. I'm assuming it will use its usual mechanism for JS exceptions, which does keep a stack trace that you can access from JS code. Rethrowing rather than a new throw might keep that one intact. It doesn't help with the C++ state, but it's still better than nothing.

Sep 15 2017, 4:56 PM
dschuff added a comment to D37942: [WebAssembly] Restore __builtin_wasm_rethrow builtin.

Or you could just call it a revert of the previous patch, if you wanted.

Sep 15 2017, 4:54 PM
dschuff accepted D37942: [WebAssembly] Restore __builtin_wasm_rethrow builtin.
Sep 15 2017, 4:53 PM
dschuff added a comment to D37886: [WebAssembly] Add support for naming wasm data segments.

So are we going to say that the mere existence of segment names tells the linker it should merge them as though they were separate data sections? (or, is there any other possible use?) Is there other potential per-MCSection metadata we'd want that would make this metadata end up more generic?

Sep 15 2017, 3:24 PM
dschuff added a comment to D37931: Remove __builtin_wasm_rethrow builtin.

So, it just occurred to me; we don't have 2-phase unwinding, which is useful to get good stack traces for debugging (because if you have unhandled exceptions you can abort before the second phase, rather than after the stack has been unwound). If the VM has an exception object from the original throw, it should have a stack trace attached to it. If we rethrow rather than throwing anew each time, then it will be preserved. So could we try to use rethrow instead of throw where possible? If we think it even might be feasible, it might be worth leaving rethrow in for now.

Sep 15 2017, 3:06 PM
dschuff accepted D37876: [WebAssembly] MC: Create wasm data segments based on MCSections.
Sep 15 2017, 12:49 PM
dschuff added a comment to D37876: [WebAssembly] MC: Create wasm data segments based on MCSections.

Should the patch title be "create data *segments* based on MCSections"?

Sep 15 2017, 10:11 AM
dschuff accepted D37875: [WebAssembly] MC: Pass ArrayRefs rather than SmallVectors.
Sep 15 2017, 8:57 AM

Sep 14 2017

dschuff added a comment to D37834: [WebAssembly] Use a separate wasm data segment for each global symbol.

Oh also a nit: It's spelled 'separate' :D

Sep 14 2017, 3:35 PM
dschuff accepted D37834: [WebAssembly] Use a separate wasm data segment for each global symbol.
Sep 14 2017, 3:34 PM
dschuff added a comment to D37869: [WebAssembly] Remove default -fdata-sections.

Do wer have any tests with global vars that have an explicit section? We should have one, that tests that they get merged into a segment in the object file.

Sep 14 2017, 2:59 PM
dschuff added a comment to D37831: [WebAssembly] Don't pass -ffunction-section/-fdata-sections by default.

I think the reasoning was really just that code size reduction is even more important on wasm than other platforms, and because it's a new ABI there's no one depending on the default behavior that would break.

Sep 14 2017, 2:44 PM
dschuff added a comment to D37831: [WebAssembly] Don't pass -ffunction-section/-fdata-sections by default.

Shouldn't the LLVM-side tests be using the LLVM utils and not clang? Or did you mean lld tests, or waterfall tests or what?

Sep 14 2017, 11:10 AM
dschuff added inline comments to D37834: [WebAssembly] Use a separate wasm data segment for each global symbol.
Sep 14 2017, 11:05 AM

Sep 13 2017

dschuff added a comment to D37831: [WebAssembly] Don't pass -ffunction-section/-fdata-sections by default.

I guess the reason for removing this is that we don't really have an analogue of different text sections in the wasm object format, but that dead function stripping can be done by any linker smart enough to link wasm at all. But this isn't necessarily true of data sections though, is it?

Sep 13 2017, 4:03 PM

Sep 12 2017

dschuff committed rL313101: [WebAssembly] Add sign extend instructions from atomics proposal.
[WebAssembly] Add sign extend instructions from atomics proposal
Sep 12 2017, 5:30 PM
dschuff closed D37603: [WebAssembly] Add sign extend instructions from atomics proposal by committing rL313101: [WebAssembly] Add sign extend instructions from atomics proposal.
Sep 12 2017, 5:30 PM
dschuff added a comment to D37603: [WebAssembly] Add sign extend instructions from atomics proposal.

Ping!

Sep 12 2017, 9:44 AM
dschuff accepted D37633: [WebAssembly] Remove flags from MCSectionWasm.
Sep 12 2017, 8:52 AM

Sep 7 2017

dschuff updated the diff for D37603: [WebAssembly] Add sign extend instructions from atomics proposal.
  • remove spurious change
Sep 7 2017, 4:22 PM
dschuff created D37603: [WebAssembly] Add sign extend instructions from atomics proposal.
Sep 7 2017, 4:19 PM

Sep 6 2017

dschuff accepted D37497: [WebAssembly] Only treat imports/exports as symbols when reading relocatable object files.

otherwise LGTM

Sep 6 2017, 1:38 PM

Sep 5 2017

dschuff added inline comments to D37497: [WebAssembly] Only treat imports/exports as symbols when reading relocatable object files.
Sep 5 2017, 9:03 PM
dschuff added inline comments to D37497: [WebAssembly] Only treat imports/exports as symbols when reading relocatable object files.
Sep 5 2017, 5:03 PM

Sep 1 2017

dschuff accepted D37385: [WebAssembly] Update reloction names to match spec.
Sep 1 2017, 10:28 AM
dschuff accepted D37384: [WebAssembly] Fix getSymbolValue for exported globals.
Sep 1 2017, 10:21 AM

Aug 31 2017

dschuff added a comment to D37359: [WebAssembly] Fix getSymbolValue() for data symbols.

but "Fixes PRNNN" is fine to indicate that a bug is fixed, although now that I think of it, a real link (in either form) is probably even better. Either way there's no auto-linking. Not everyone even uses LLVM's bug tracker anyway.

Aug 31 2017, 2:56 PM
dschuff committed rL312287: [WebAssembly] Refactor load ISel tablegen patterns into classes.
[WebAssembly] Refactor load ISel tablegen patterns into classes
Aug 31 2017, 2:55 PM
dschuff closed D37345: [WebAssembly] Refactor load ISel tablegen patterns into classes by committing rL312287: [WebAssembly] Refactor load ISel tablegen patterns into classes.
Aug 31 2017, 2:55 PM
dschuff added a comment to D37359: [WebAssembly] Fix getSymbolValue() for data symbols.

It's just a bug number. PR I guess is a "Problem Report"? Also https://llvm.org/PR34392 is a shortcut to https://bugs.llvm.org/show_bug.cgi?id=34392

Aug 31 2017, 2:55 PM
dschuff added a comment to D37359: [WebAssembly] Fix getSymbolValue() for data symbols.

ah we raced. LGTM :)

Aug 31 2017, 2:45 PM
dschuff accepted D37359: [WebAssembly] Fix getSymbolValue() for data symbols.

Cool, I guess this fixes PR34392, you should put that in the commit description too.

Aug 31 2017, 2:45 PM
dschuff accepted D37358: [WebAssembly] Validate exports when parsing object files.
Aug 31 2017, 2:41 PM
dschuff updated the diff for D37345: [WebAssembly] Refactor load ISel tablegen patterns into classes.
  • address comments: remove atomic rules with constant offsets
Aug 31 2017, 2:30 PM
dschuff added inline comments to D37345: [WebAssembly] Refactor load ISel tablegen patterns into classes.
Aug 31 2017, 2:24 PM
dschuff created D37345: [WebAssembly] Refactor load ISel tablegen patterns into classes.
Aug 31 2017, 11:12 AM

Aug 30 2017

dschuff committed rL312163: [WebAssembly] Update debug info test after r312144.
[WebAssembly] Update debug info test after r312144
Aug 30 2017, 12:58 PM
dschuff committed rL312145: [WebAssembly] Add target feature for atomics.
[WebAssembly] Add target feature for atomics
Aug 30 2017, 11:09 AM
dschuff closed D37300: [WebAssembly] Add target feature for atomics by committing rL312145: [WebAssembly] Add target feature for atomics.
Aug 30 2017, 11:09 AM
dschuff updated the diff for D37300: [WebAssembly] Add target feature for atomics.
  • address review comments
Aug 30 2017, 9:56 AM
dschuff added inline comments to D37300: [WebAssembly] Add target feature for atomics.
Aug 30 2017, 9:56 AM
dschuff updated the diff for D37300: [WebAssembly] Add target feature for atomics.
  • revert extra change
Aug 30 2017, 9:09 AM
dschuff updated the summary of D37300: [WebAssembly] Add target feature for atomics.
Aug 30 2017, 9:07 AM
dschuff created D37300: [WebAssembly] Add target feature for atomics.
Aug 30 2017, 9:06 AM

Aug 24 2017

dschuff accepted D37073: [WebAssembly] FastISel : Bail to SelectionDAG for constexpr calls.
Aug 24 2017, 12:26 PM
dschuff added a comment to D37073: [WebAssembly] FastISel : Bail to SelectionDAG for constexpr calls.

Also looking back and my previous comment,

Aug 24 2017, 11:03 AM
dschuff accepted D37100: [WebAssembly] Update GCC test suite failure expectations.
Aug 24 2017, 8:46 AM

Aug 23 2017

dschuff added a comment to D37073: [WebAssembly] FastISel : Bail to SelectionDAG for constexpr calls.

OK nevermind, GlobalVariable doesn't make sense because you have to load it, and similarly for GEP. We could probably do inttoptr though, and maybe select, to get a function pointer and call that.

Aug 23 2017, 10:33 AM
dschuff added a comment to D37073: [WebAssembly] FastISel : Bail to SelectionDAG for constexpr calls.

Are there any other constants/constexprs besides bitcast that we might encounter, that we should test? Looking at the constexprs, I don't see too much that looks likely. Maybe GEP? I guess you can indirectly call a GlobalVariable, which ought to look the same as a local function pointer, maybe we should have a test for that too (presumably dag isel handles that too).
Also that this patch really does is just bail from fast-isel when the callee is a constant (which is fine), and that has the effect of causing calls of bitcasts to be lowered as direct calls.

Aug 23 2017, 10:03 AM
dschuff accepted D37070: [WebAssembly] Fix overflow for input without version.

Thanks!

Aug 23 2017, 9:19 AM

Aug 15 2017

dschuff committed rL310981: [WebAssembly] Remove infinite loop from reg-stackify test.
[WebAssembly] Remove infinite loop from reg-stackify test
Aug 15 2017, 5:51 PM

Aug 14 2017

dschuff accepted D36595: [WebAssembly] Remove invalid lld arguments.
Aug 14 2017, 2:26 PM

Jul 21 2017

dschuff added a comment to D35522: Move Runtime libcall definitions to a .def file.

Actually, maybe an even better idea:
getLibcallName() returns a StringRef and asserts rather than returning a failure code. getLIbcallNameIfAvailable() returns an Expected<StringRef> and can fail.
The former matches 90% of the existing uses. This would let us move away from char* for the external symbol APIs (assuming that's a good thing) but most of the existing uses wouldn't need to actually change, and those that check failure would still be simple.

Jul 21 2017, 10:10 AM
dschuff added a comment to D35522: Move Runtime libcall definitions to a .def file.

So it looks like switching the API away from char*-based would be, maybe not super-trivial, but probably not too bad. It's interlinked essentially with other APIs relating to external symbols:
mostly SelectionDAG::getExternalSymbol(), and also MachineInstrBuilder::addExternalSymbol() but those could be converted too, and I don't think I'd mind doing it as general cleanup. I wonder if Expected<StringRef> is a little heavyweight though. It seems to have a really strong convention that the result should be checked first, and most use cases just don't care (arguably they could assert, but requiring that would add a lot of boilerplate asserts for probably not a lot of benefit). StringRef by itself (with empty string taking the place of the current use of nullptr) wouldn't be too bad, although having the empty string as a sort of sentinel value only used by a small fraction of the uses doesn't seem ideal either. I might still be inclined to prefer that over Expected though. Any thoughts?

Jul 21 2017, 9:54 AM