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 (267 w, 2 d)

Recent Activity

Fri, Feb 16

dschuff added a comment to D43365: [WebAssembly] MC: Make explicit our current lack of support for relocations against unnamed temporary symbols..

Yeah come to think of it, I thought we just failed isel for blockaddress?

Fri, Feb 16, 9:26 AM
dschuff accepted D43365: [WebAssembly] MC: Make explicit our current lack of support for relocations against unnamed temporary symbols..
Fri, Feb 16, 8:47 AM

Mon, Feb 12

dschuff accepted D43212: [WebAssembly] Update ADT/TripleTest.cpp now that default file format has changed.
Mon, Feb 12, 3:46 PM
dschuff accepted D43205: [WebAssembly] Fix casting MCSymbol to MCSymbolWasm on ELF.
Mon, Feb 12, 12:31 PM
dschuff added inline comments to D43205: [WebAssembly] Fix casting MCSymbol to MCSymbolWasm on ELF.
Mon, Feb 12, 12:17 PM

Fri, Feb 9

dschuff added a comment to D42576: [test-suite] Add prototypes to functions in Olden and MiBench.
  • My reading of the C standard is that using a prototype less declaration and defining that as a vararg functions is invalid in C. If you find bugs in this area we should fix them.

Interesting, I'll look closer at that. We haven't gotten any complaints about it so far.

  • This patch simply removes all prototype less functions which is not fine.

Sorry, in case I wasn't clear, I agree that these tests are valid C and we should not change them.

  • The llvm IR coming out of clang appears to mark all prototype less functions as varargs which from what I suspect makes it impossible to implement varargs different from prototype-less functions. But that is a problem in clang/IR, it doesn't change the fact that the tests are valid C.

Yeah, you can sort of cheat because in C, vararg functions must have at least one non-vararg parameter (at least AFAIK no C standard implemented by Clang allows that). So you can tell a real vararg function declare i32 @foo(i32, ...) from a non-prototype function declare i32 @foo(...). I suspect that only works for C though.

Fri, Feb 9, 12:13 PM
dschuff added a comment to D42576: [test-suite] Add prototypes to functions in Olden and MiBench.

I'll just add that what we do in WebAssembly right now for unprototyped functions is to codegen them using the fixed-arg calling convention; at link-time if there are any mismatches between the definition and any calls, then the program has UB, and the resulting WebAssembly module will be invalid. I think this currently has the problem that if the definition does turn out to be vararg, then we have generated an invalid module even though the program is correct (I consider that a bug or shortcoming in our toolchain though). In any case, we do aim to support correct programs with prototypeless functions. If the test suite has non-vararg cases where there are mismatches between the callsites and/or the definition (I have seen that in some other test/benchmark suites), then even though that works on most X86 and ARM ABIs that I know of it's still UB and I'd be in favor of removing it from LLVM's test suite.

Fri, Feb 9, 11:22 AM

Thu, Feb 8

dschuff added a comment to D42435: [WebAssembly] Rely on default file format in test code. NFC..

I like this idea, did you abandon this CL in favor of something else?

Thu, Feb 8, 11:23 AM

Mon, Feb 5

dschuff added a comment to D42788: [SDAG] Legalize all CondCodes by inverting them and/or swapping operands.

So, it turns out that this actually does change functionality on targets (such as WebAssembly) that use the expansion for unordered comparisons because it tries inverting the condition before falling all the way through to line 1680. This actually results in better code for wasm; see https://reviews.llvm.org/rL324305

Mon, Feb 5, 5:47 PM
dschuff updated subscribers of rL324305: [WebAssembly] Fix test expectations after r324274.

@sunfish This is really just a change-detector test for the cond code expansion behavior that changed in https://reviews.llvm.org/D42788. Should we consider loosening it so that it just tests that we used the expansion? OTOH it's a bit concerning that that change didn't affect any other tests (!)

Mon, Feb 5, 5:26 PM
dschuff committed rL324305: [WebAssembly] Fix test expectations after r324274.
[WebAssembly] Fix test expectations after r324274
Mon, Feb 5, 5:23 PM
dschuff accepted D42869: [WebAssembly] Remove DataSize from linking metadata.
Mon, Feb 5, 1:54 PM

Fri, Feb 2

dschuff added a comment to D42867: [WebAssembly] Add __data_end linker-synthetic symbol.

I do like the idea of having a linker-generated symbol instead of metadata for _end (or _wasm_end or whatever) and it's something that is unlikely to be broken in the future. So I think we should do that. But I do agree that specifying all these things would limit our ability to improve the layout in the future, especially wrt the order of the static/heap/stack bits. Since we have more constraints on our memory space than ELF (e.g. expanding only at the top instead of mmap), we might want to retain that flexibility, or even make things more dynamic or configurable, which would probably argue against matching the behavior of ELF here. We could consider having separate start/end markers for particular sections instead of having implicit ordering, if there's a use case for that.

Fri, Feb 2, 4:17 PM

Wed, Jan 31

dschuff added a comment to D42095: [WebAssembly] Output provisional table to match LLD relocatable output.

This change does not effect lld because the wasm "table" in
object file format is purely cosmetic and lld ignores its
contents.

Wed, Jan 31, 10:49 AM
dschuff added a comment to D37831: [WebAssembly] Don't pass -ffunction-section/-fdata-sections .

After a little discussion about this and the -gc-sections linker flag, the augments for the tools having sensible defaults seem to be winning.

Wed, Jan 31, 9:21 AM
dschuff accepted D37831: [WebAssembly] Don't pass -ffunction-section/-fdata-sections .
Wed, Jan 31, 9:21 AM

Tue, Jan 30

dschuff added a comment to D42511: [WebAssembly] Add support for --gc-sections.

I think LLD should probably default to no gc, and in general to minimal/most simple options, and the smarts should be in the driver. I think this matches other targets, and I think it also matches (or at least, should match) the way we have options the frontend/backend vs the driver; i.e. IIRC the driver has logic to pass -ffunction-sections and other codegen options to the frontend. So basically all the special logic goes in one place.

Tue, Jan 30, 9:26 AM

Tue, Jan 23

dschuff accepted D40526: [WebAssembly] Change size_t to `unsigned long`.

LGTM FTR, as long as we coordinate when this lands with emscripten.

Tue, Jan 23, 2:45 PM
dschuff accepted D42440: [WebAssembly] MC: Use inline triple in test bitcode files.
Tue, Jan 23, 2:44 PM
dschuff added a comment to D42433: [WebAssembly] Update wasm target triple now that its the default.

I was going to say that most llc tests in LLVM don't use mtriple and that we should be consistent with that, but a quick grep suggests that something like 65% of the RUN lines with llc actually do contain mtriple. So I guess we could go either way. I think we still probably want to keep the triple in the bitcode (it is more convenient to run just llc, but only marginally so), in which case mtriple would be redundant. But I don't have strong opinions.

Tue, Jan 23, 12:23 PM

Jan 22 2018

dschuff added a comment to D42226: [WebAssembly] Add test expectations for gcc C++ tests (gcc/testsuite/g++.dg).

These are just the compile failures :) There's also https://github.com/WebAssembly/waterfall/pull/316/files#diff-a6006086cef2fc4f685647d1fe006766

Jan 22 2018, 3:40 PM

Jan 19 2018

dschuff committed rL323012: [WebAssembly] Fix MSVC build.
[WebAssembly] Fix MSVC build
Jan 19 2018, 4:05 PM
dschuff accepted D42284: [WebAssembly] MC: Start table at offset 1 rather than 0.
Jan 19 2018, 10:57 AM
dschuff committed rL322971: [WebAssembly] Fix libcall signature lookup.
[WebAssembly] Fix libcall signature lookup
Jan 19 2018, 9:49 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Jan 19 2018, 9:49 AM
dschuff updated the diff for D42271: [WebAssembly] Fix libcall signature lookup.
  • use StringRef::withNullAsEmpty() and clang-format
Jan 19 2018, 8:44 AM
dschuff added inline comments to D42271: [WebAssembly] Fix libcall signature lookup.
Jan 19 2018, 8:44 AM

Jan 18 2018

dschuff added a reviewer for D42271: [WebAssembly] Fix libcall signature lookup: aheejin.
Jan 18 2018, 4:47 PM
dschuff updated the diff for D42271: [WebAssembly] Fix libcall signature lookup.
  • fix build
Jan 18 2018, 4:43 PM
dschuff created D42271: [WebAssembly] Fix libcall signature lookup.
Jan 18 2018, 4:26 PM
dschuff accepted D42226: [WebAssembly] Add test expectations for gcc C++ tests (gcc/testsuite/g++.dg).
Jan 18 2018, 4:26 PM

Jan 17 2018

dschuff committed rL322802: [WebAssembly] Remove duplicated RTLIB names.
[WebAssembly] Remove duplicated RTLIB names
Jan 17 2018, 5:18 PM
dschuff closed D35592: [WebAssembly] Remove duplicated RTLIB names.
Jan 17 2018, 5:18 PM
dschuff updated the diff for D35592: [WebAssembly] Remove duplicated RTLIB names.
  • move types into anon namespace (and static stuff too just for consistency
Jan 17 2018, 5:11 PM
dschuff added a comment to D42030: [WebAssembly] Define __heap_base global.

I don't see that. How the underlying system allocates memory, and how it's split up for application use, don't have to relate at all. Exactly the same reasoning would say, "well x86 uses 4K pages so it doesn't make sense for malloc to subdivide that for a 10-byte allocation". I do agree though that it's odd/unusual and also awkward for mmap not to reflect the kernel's page size - but odd doesn't make it wrong, or inefficient.

Jan 17 2018, 4:21 PM
dschuff updated subscribers of D35592: [WebAssembly] Remove duplicated RTLIB names.

Ping! This refactoring makes less work for anyone changing libcalls... any review takers?

Jan 17 2018, 3:31 PM
dschuff updated the diff for D35592: [WebAssembly] Remove duplicated RTLIB names.
  • Merge branch 'master' into wasm_rtlib_def
  • fix conflict
Jan 17 2018, 3:30 PM
dschuff added a comment to D42030: [WebAssembly] Define __heap_base global.

It seems like the real issue is that because it's meant for Linux, musl assumes the existence of Linux's mmap. On traditional architectures, there's a machine page size, which is also the granularity of mmap on Linux (technically you can have some kinds of mappings that don't end on page boundaries but those pages will just be fragmented). There's really no reason to do anything not on page boundaries, adn 4k is a pretty small page. The with wasm is that there's really no such thing as mmap for wasm (or I should say, for any wasm embedding, since mmap is an OS feature rather than an architecture feature). mmap is a strange swiss army knife, and currently the only functionality is to grow the memory at the end; If and when additional functionality is added (file mapping, protection, unmapped space, etc) that would be as separate calls. So mmap must always be emulated and if it is emulated then it's really just another layer of allocation. To me it makes much more sense to have one allocator (malloc) which knows directly about how wasm memory is managed (instead of assuming mmap's behavior which will always be a fiction, at least in JS-wasm). Since the real behavior is more or less brk() it makes sense to use that logic in existing allocators.
Likewise, because memory can never be allocated from the underlying system in increments of less than 64k, it doesn't really make much sense to pretend otherwise by implementing an mmap for malloc to use.

Jan 17 2018, 2:20 PM

Jan 16 2018

dschuff added inline comments to D42030: [WebAssembly] Define __heap_base global.
Jan 16 2018, 9:06 AM
dschuff added a comment to D42024: [WebAssembly] Create synthetic __dso_handle symbol.

Couldn't this just be in crtbegin.c? Then there wouldn't have to be custom linker logic for it.

Jan 16 2018, 9:03 AM

Jan 11 2018

dschuff accepted D41937: [WebAssembly] supports -stdlib=libc++ switch.

LGTM, but does it need to be rebased after the -allow-undefined-file change?

Jan 11 2018, 2:47 PM
dschuff added a comment to D41966: [WebAssembly] Add -lc++-abi when linking C++ programs.

Cool, let's go with D41937, it has a proper diagnostic, and a test!

Jan 11 2018, 2:46 PM
dschuff added inline comments to D41941: [WebAssembly] Change int_fast16_t to 32-bit.
Jan 11 2018, 10:36 AM
dschuff added inline comments to D41941: [WebAssembly] Change int_fast16_t to 32-bit.
Jan 11 2018, 10:08 AM
dschuff added inline comments to D41941: [WebAssembly] Change int_fast16_t to 32-bit.
Jan 11 2018, 9:21 AM

Jan 10 2018

dschuff accepted D41923: [WebAssembly] Remove `-allow-undefined-file wasm.syms` from linker args.

The code LGTM. If there are objections to the scheme overall they can go in tool-conventions.

Jan 10 2018, 4:30 PM

Jan 9 2018

dschuff added a comment to D35592: [WebAssembly] Remove duplicated RTLIB names.

Coming back to this patch, in order to remove the tight coupling between the arrays in WebAssemblyRuntimeLibcallSignatures.cpp and the list of libcalls in the def file.

Jan 9 2018, 3:02 PM
dschuff updated the diff for D35592: [WebAssembly] Remove duplicated RTLIB names.
  • fix conflicts
Jan 9 2018, 2:38 PM
dschuff updated the diff for D35592: [WebAssembly] Remove duplicated RTLIB names.
  • Merge branch 'master' into wasm_rtlib_def
Jan 9 2018, 2:22 PM
dschuff accepted D41840: [WebAssembly] MC: Use zero for provisional value of undefined symbols.
Jan 9 2018, 12:56 PM
dschuff accepted D41878: [WebAssembly] Update YAML in tests to match LLVM change.
Jan 9 2018, 12:54 PM
dschuff accepted D41877: [WebAssembly] Explicitly specify function/global index space in YAML.
Jan 9 2018, 12:54 PM
dschuff committed rL322105: [WebAssembly] Update libcall signature lists.
[WebAssembly] Update libcall signature lists
Jan 9 2018, 11:06 AM

Dec 15 2017

dschuff accepted D41265: [WebAssembly] Return ArrayRef's rather than const std::vector&.
Dec 15 2017, 2:17 PM

Dec 7 2017

dschuff committed rL320121: Revert "[WebAssemby] Support main functions with alternate signatures.".
Revert "[WebAssemby] Support main functions with alternate signatures."
Dec 7 2017, 4:40 PM

Dec 5 2017

dschuff committed rL319870: [WebAssembly] Only emit stack pointer delcaration in BinFormatWasm assembly.
[WebAssembly] Only emit stack pointer delcaration in BinFormatWasm assembly
Dec 5 2017, 5:39 PM
dschuff committed rL319865: [WebAssembly] Fix test breakage from r319810.
[WebAssembly] Fix test breakage from r319810
Dec 5 2017, 5:03 PM

Nov 29 2017

dschuff added inline comments to D40559: Wasm entrypoint changes #2 (export entrypoint in "start" section) APPLY AFTER D40724.
Nov 29 2017, 10:10 AM

Nov 28 2017

dschuff added a comment to D40559: Wasm entrypoint changes #2 (export entrypoint in "start" section) APPLY AFTER D40724.

tl;dr: I think I agree with @sunfish

Nov 28 2017, 2:44 PM

Nov 9 2017

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

Nov 9 2017, 3:47 PM

Nov 8 2017

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

Nov 7 2017

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.

Nov 7 2017, 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.

Nov 7 2017, 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.

Nov 7 2017, 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.

Nov 7 2017, 10:53 AM

Oct 27 2017

dschuff accepted D39354: [WebAssembly] Add crt1.o when calling lld.
Oct 27 2017, 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