Page MenuHomePhabricator

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 (323 w, 3 d)

US-Pacific timezone.

Recent Activity

Tue, Mar 19

dschuff added inline comments to D48345: [WebAssembly] Fix unwind destination mismatches in CFG stackify.
Tue, Mar 19, 3:22 PM · Restricted Project

Mon, Mar 18

dschuff accepted D59519: [WebAssembly] Lower SIMD nnan setcc nodes.

to make the change even clearer, you could just say in the commit message that it just adds the missing non-equality opcodes.

Mon, Mar 18, 5:36 PM · Restricted Project
dschuff accepted D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..
Mon, Mar 18, 2:18 PM · Restricted Project
dschuff accepted D59447: [WebAssembly] Add immarg attribute to intrinsics.
Mon, Mar 18, 1:37 PM · Restricted Project
dschuff accepted D59445: Restore comment regarding why Reloc::PIC_ can't be PIC.
Mon, Mar 18, 12:58 PM · Restricted Project
dschuff accepted D59448: [WebAssembly] Change wasm.throw's first argument to an immediate.

LGTM; I wonder if it makes sense to have predefined macro for the C++ tag (or perhaps just a regular macro for use in libcxxabi?)

Mon, Mar 18, 12:55 PM · Restricted Project

Fri, Mar 15

dschuff added a comment to D54647: [WebAssembly] Initial implementation of PIC code generation.

wow that's a very large meme.

Fri, Mar 15, 5:03 PM · Restricted Project
dschuff accepted D54647: [WebAssembly] Initial implementation of PIC code generation.
Fri, Mar 15, 5:03 PM · Restricted Project

Thu, Mar 14

dschuff added a comment to D59395: [WebAssembly] Remove unused LoadPatExternalSym patterns.

Macro shipit:

Thu, Mar 14, 5:21 PM · Restricted Project
dschuff accepted D59395: [WebAssembly] Remove unused LoadPatExternalSym patterns.

LGTM assuming that comment is right. Assuming it's correct, I don't really understand what exactly ExternalSymbol actually means though.

Thu, Mar 14, 4:39 PM · Restricted Project
dschuff accepted D59353: [WebAssembly] Use rethrow intrinsic in the rethrow block.
Thu, Mar 14, 2:30 PM · Restricted Project
dschuff accepted D59352: [WebAssembly] Make rethrow take an except_ref type argument.

otherwise LGTM

Thu, Mar 14, 2:30 PM · Restricted Project
dschuff accepted D59342: [WebAssembly] Method order change in LateEHPrepare (NFC).
Thu, Mar 14, 11:10 AM · Restricted Project

Tue, Mar 12

dschuff accepted D58914: [WebAssembly] Place 'try' and 'catch' correctly wrt EH_LABELs.

So by "correctly" you mean that the TRY goes before the EH_LABEL rather than between the label and the call, and the CATCH goes after the label rather than at the top of the BB?

Tue, Mar 12, 11:00 AM · Restricted Project

Thu, Mar 7

dschuff added inline comments to D59007: [WebAssembly] Use named operands to identify loads and stores.
Thu, Mar 7, 11:08 AM · Restricted Project

Tue, Mar 5

dschuff accepted D58953: [WebAssembly] Disable MachineBlockPlacement pass.

LGTM with the comment suggestion.

Tue, Mar 5, 10:27 AM · Restricted Project

Tue, Feb 26

dschuff added a comment to D58486: [WebAssembly] Delete ThrowUnwindDest map from WasmEHFuncInfo.

still LGTM

Tue, Feb 26, 2:09 PM · Restricted Project
dschuff accepted D58605: [WebAssembly] Fix ScopeTops info in CFGStackify for EH pads.
Tue, Feb 26, 2:06 PM · Restricted Project
dschuff accepted D58591: [WebAssembly] Remove unnecessary instructions after TRY marker placement.
Tue, Feb 26, 1:59 PM · Restricted Project

Mon, Feb 25

dschuff added inline comments to D58591: [WebAssembly] Remove unnecessary instructions after TRY marker placement.
Mon, Feb 25, 1:33 PM · Restricted Project
dschuff accepted D58562: [WebAssembly] Improve readability of EH tests.

LGTM, this is a nice improvement.

Mon, Feb 25, 1:15 PM · Restricted Project
dschuff accepted D58519: [WebAssembly] Fix a bug deleting instruction in a ranged for loop.
Mon, Feb 25, 1:08 PM · Restricted Project

Fri, Feb 22

dschuff accepted D58472: [WebAssembly] Remove unneeded MCSymbolRefExpr variants.

Makes sense. I think the thing that has changed compared to when we started is that symbol types like function, global, and event are more first-class in the wasm object format. Anyway, this change seems fine. I assume the asm parser already isn't using these annotations and we still round-trip?

Fri, Feb 22, 2:06 PM · Restricted Project
dschuff added a comment to D58486: [WebAssembly] Delete ThrowUnwindDest map from WasmEHFuncInfo.

Makes sense.
Is EHPadUnwindMap analogous to any of the similarly-named data structures for WinEH? If so, that would seem to make it even less likely to accidentally break, since any change that breaks it would also break WinEH.

Fri, Feb 22, 1:57 PM · Restricted Project
dschuff accepted D58486: [WebAssembly] Delete ThrowUnwindDest map from WasmEHFuncInfo.

You mention that this is a bugfix. what's the bug that is fixed? An optimization not keeping the map up to date?

Fri, Feb 22, 1:34 PM · Restricted Project
dschuff accepted D58417: [WebAssembly] MC: Handle aliases of aliases.
Fri, Feb 22, 11:44 AM · Restricted Project
dschuff added a comment to D58472: [WebAssembly] Remove unneeded MCSymbolRefExpr variants.

IIRC this code goes back to when we were piggybacking on ELF, and is more-or-less there to match how those variants are used on ELF. I don't see any asm parser changes in this CL, but I could imagine that they would exist so that the assembler could know what kind of reloc to use even without any context or special knowledge of symbol types; does this change affect how asm is parsed? Maybe we've already added enough extra intelligence to the assembler that we don't need them.

Fri, Feb 22, 11:37 AM · Restricted Project

Thu, Feb 21

dschuff accepted D58319: [WebAssembly] Remove getBottom function from CFGStackify (NFC).
Thu, Feb 21, 5:37 PM · Restricted Project
dschuff added a comment to D58488: [WebAssembly] Default to a reasonable signature in WebAssemblyAddMissingPrototypes.

LGTM

Thu, Feb 21, 3:39 PM · Restricted Project

Feb 15 2019

dschuff accepted D58304: [WebAssembly] Warn but don't error on conflicting uses of prototype-less functions.

By "pick one at random" you mean we are rewriting one of the calls to drop or add parameters, right? And the one we pick is just the first one we happen to encounter?

Feb 15 2019, 2:57 PM · Restricted Project

Feb 12 2019

dschuff added a comment to D57938: [WebAssembly] Update MC for bulk memory.

Without the context of MC here there's no indication what kind of flags the .init_flags directive refers to. Maybe call it init_segment_flags?

Feb 12 2019, 4:44 PM · Restricted Project

Feb 7 2019

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

@quinripa IIUC this patch was committed a year ago, so these symbols should be working?

Feb 7 2019, 3:22 PM · Restricted Project
dschuff accepted D57800: [WebAssembly] LTO: Set POSIX thread model when linking with -shared-memoey.

Yeah, based on our conversations (e.g. in D57874) we should go ahead with this.

Feb 7 2019, 3:18 PM · Restricted Project
dschuff added a comment to D57874: [WebAssembly] Make thread-related options consistent.

Oh I guess another option would just be to pin all 3 flags together here, but since -pthread sets a preprocessor define and may also affect linker behavior, I think it's fine to allow atomic codegen without setting -pthread.

Feb 7 2019, 10:46 AM · Restricted Project, Restricted Project
dschuff added a comment to D57874: [WebAssembly] Make thread-related options consistent.

So this CL has the effect that setting -pthreads will also set -matomics.
Currently as you mentioned we have the problem that we can't make our current logic of "do we lower away the atomics" be controlled by the target features because it's done at pass config time and not codegen time (where there's no access to the per-function subtarget).
Also IIUC on ARM, it's -mthread-model that controls generation of atomic instructions, so it makes sense that -mthread-model does the same thing for Wasm. But it's also true that for wasm we use -m<feature> to enable codegen for a particular wasm feature, so it would be good to make -matomics do the same for consistency.
So, can we just pin those 2 flags together here? i.e. setting one will just cause the other to be set?
(I guess we'd also need consistency check so that if someone does something like -mthread-model=posix -mno-atomics we either throw an error or make it so that one of those always overrides the other).

Feb 7 2019, 10:44 AM · Restricted Project, Restricted Project

Feb 6 2019

dschuff added a comment to D57861: [WebAssembly] Expand symbols shown by llvm-objdump --symbols.

Does this affect llvm-objdump or just llvm-readobj, or both (I don't see any tests here with llvm-objdump but the CL description mentions it)

Feb 6 2019, 5:17 PM · Restricted Project
dschuff accepted D57864: [WebAssembly] Add symbol flag to the binary format corresponding to llvm.used.
Feb 6 2019, 5:14 PM · Restricted Project
dschuff added a comment to D57800: [WebAssembly] LTO: Set POSIX thread model when linking with -shared-memoey.

I think we should change the logic in the backend to use the atomics target feature rather than the thread model to determine whether to run the LowerAtomics pass. Then this won't be necessary since the functions will have the appropriate attribute.

Feb 6 2019, 11:07 AM · Restricted Project

Feb 4 2019

dschuff added inline comments to D57713: [WebAssembly] Make disassembler always emit most canonical name..
Feb 4 2019, 2:30 PM · Restricted Project
dschuff accepted D57695: [llvm-readobj] Report more WebAssembly symbol info.
Feb 4 2019, 2:21 PM · Restricted Project

Feb 1 2019

dschuff accepted D57610: [WebAssembly] Remove redundant namespaces qualifiers. NFC..
Feb 1 2019, 1:28 PM · Restricted Project
dschuff accepted D57546: [WebAssembly] Make segment/size/type directives optional in asm.

otherwise LGTM too

Feb 1 2019, 11:13 AM · Restricted Project
dschuff added inline comments to D57546: [WebAssembly] Make segment/size/type directives optional in asm.
Feb 1 2019, 11:13 AM · Restricted Project
dschuff added inline comments to D57546: [WebAssembly] Make segment/size/type directives optional in asm.
Feb 1 2019, 11:10 AM · Restricted Project
dschuff added a comment to D57495: [WebAssembly] Add bulk memory target feature.

It's always been possible because clang and LLVM have always just been in different trunks in the same SVN repo. AFAIK it's never been disallowed but people rarely do it.

Feb 1 2019, 10:24 AM · Restricted Project

Jan 31 2019

dschuff added inline comments to D57498: [WebAssembly] memory.copy.
Jan 31 2019, 1:20 PM · Restricted Project
dschuff accepted D57538: [WebAssembly] MC: Fix type of function aliases.

LGTM. Can you put a little more detail in the commit message (e.g. something like what you wrote in the comment) to make it easier to see in a log (without having to look at the code or the PR)?

Jan 31 2019, 1:14 PM · Restricted Project

Jan 30 2019

dschuff added a comment to D57480: [WebAssembly] Remove TODO on wasm.extract.exception intrinsic (NFC).

Just noticed, but the discussion should maybe have gone into a comment on Phab rather than the commit message itself...

Jan 30 2019, 4:02 PM
dschuff accepted D57480: [WebAssembly] Remove TODO on wasm.extract.exception intrinsic (NFC).

yeah, seems fine. we can always change our mind in the future even in the absence of a TODO :)

Jan 30 2019, 3:35 PM
dschuff accepted D57477: [WebAssembly] MC: Use WritePatchableLEB helper function. NFC..
Jan 30 2019, 2:42 PM
dschuff accepted D57421: [WebAssembly] Restore stack pointer right after catch instruction.
Jan 30 2019, 1:40 PM

Jan 29 2019

dschuff accepted D57134: [WebAssembly] Exception handling: Switch to the new proposal.
Jan 29 2019, 4:52 PM

Jan 28 2019

dschuff added inline comments to D57134: [WebAssembly] Exception handling: Switch to the new proposal.
Jan 28 2019, 5:17 PM

Jan 25 2019

dschuff added a comment to D57134: [WebAssembly] Exception handling: Switch to the new proposal.

overall it looks good; it's nice that this version is simpler.

Jan 25 2019, 3:15 PM

Jan 24 2019

dschuff added a comment to D57134: [WebAssembly] Exception handling: Switch to the new proposal.

haven't quite gotten through it but here's what I have so far. Looking pretty good overall though.

Jan 24 2019, 5:37 PM
dschuff accepted D57155: [WebAssembly] Add a __wasi__ target macro.

LGTM; don't forget to include all the context in the future

Jan 24 2019, 11:31 AM
dschuff accepted D57160: [WebAssembly] Add an import_module function attribute.
Jan 24 2019, 11:30 AM
dschuff accepted D57153: [WebAssembly] Factor commonality between wasm32 and wasm64 in test/Preprocessor/init.c.
Jan 24 2019, 11:28 AM
dschuff accepted D57154: [WebAssembly] Support __float128.
Jan 24 2019, 11:27 AM

Jan 17 2019

dschuff accepted D56684: [WebAssembly] Fixed objdump not parsing function headers..
Jan 17 2019, 9:32 AM

Jan 16 2019

dschuff added inline comments to D56742: [WebAssembly] Parse llvm.ident into producers section.
Jan 16 2019, 2:23 PM

Jan 15 2019

dschuff accepted D56762: [WebAssembly] Store section alignment as a power of 2.
Jan 15 2019, 5:35 PM
dschuff accepted D56758: [WebAssembly] Store section alignment as a power of 2.

This code LGTM modulo whatever version number we settle on in the tool-conventions issue.

Jan 15 2019, 4:52 PM
dschuff added a comment to D56742: [WebAssembly] Parse llvm.ident into producers section.

As a follow-on, I guess we could get ambitious and add the "language" field as well. I think the only way to do that would be to sniff the language field of the DICompileUnit metadata node (it would of course only exist if there's debug info, but doing anything else seems a bit crazytown).

Jan 15 2019, 2:33 PM
dschuff added a comment to D56684: [WebAssembly] Fixed objdump not parsing function headers..

Yeah, thinking about this a little more, I think it makes sense. IIRC the "address" (or value of a symbol, which I think is equivalent?) in the object file/linking/executable context refers to the section offset rather than memory-mapped address even for ELF, which would make our objdump consistent with other targets too. For the purposes of LLVM internals, I think you are right that having that consistency seems best; for the users, it's possible we'll want to put some more info in the final displayed output, but we can figure out how to get that once the guts are hooked up.

Jan 15 2019, 11:05 AM
dschuff committed rL351213: [WebAssembly] Update release notes.
[WebAssembly] Update release notes
Jan 15 2019, 9:58 AM
dschuff closed D56681: [WebAssembly] Update release notes.
Jan 15 2019, 9:58 AM

Jan 14 2019

dschuff added a comment to D56684: [WebAssembly] Fixed objdump not parsing function headers..

Oh I had actually forgotten that the PC offset is the offset in the module, not the offset within the function. So for objdump either way would be ok, since you could easily find what function a particular instruction is in. I guess my point is not that the formats have to match, (since e.g. you probably wouldn't be using the same code to parse both browser output and tool output?) but if you are debugging and looking at a stack trace, you should easily be able to find the same location in an objdump-produced assembly dump.... Without a calculator (annoyance of mismatched hex/dec address formats was what originally led me to add that section to the spec :) )

Jan 14 2019, 5:10 PM
dschuff added a comment to D56684: [WebAssembly] Fixed objdump not parsing function headers..

Is this actually what we want?
If I'm debugging a crash in a browser, the browser shows stack traces by function index rather than file offset, in this format
${url}:wasm-function[${funcIndex}]:${pcOffset}
(see https://webassembly.github.io/spec/web-api/index.html#conventions)
So it might be better for objdump to match that format.

Jan 14 2019, 5:05 PM
dschuff created D56681: [WebAssembly] Update release notes.
Jan 14 2019, 1:28 PM
dschuff accepted D56553: [WebAssembly] Support multilibs for wasm32 and add a wasm OS that uses it.
Jan 14 2019, 1:12 PM
dschuff added a comment to D56553: [WebAssembly] Support multilibs for wasm32 and add a wasm OS that uses it.

One difference between the multilib style and multi-arch style is whether any headers can be shared between the 2 arches. Historically at least, with bitness-flavored variants of the same architecture there were common headers used by both, and it was just the library directories that needed to be different. I've heard anecdotally that this is not the case for ARM though.

Jan 14 2019, 10:30 AM
dschuff accepted D56645: [WebAssembly] Remove old intrinsics.
Jan 14 2019, 8:55 AM

Jan 11 2019

dschuff added a comment to D56553: [WebAssembly] Support multilibs for wasm32 and add a wasm OS that uses it.

LGTM but is there really no test for this? I'd expect to see this on linker command lines or something, right? If not we should add one.

Jan 11 2019, 1:21 PM
dschuff added a comment to D56094: [WebAssembly] Fix stack pointer store check in RegStackify.

Could this CL be the cause of the newly-passing test in https://ci.chromium.org/p/wasm/builders/luci.wasm.ci/linux/2560 ?

Jan 11 2019, 1:17 PM

Jan 9 2019

dschuff added a comment to D48404: Don't modify LibFuncs in DeadArgumentElimination or ArgumentPromotion.

Just ran across this again. @jgravelle-google is this still relevant?

Jan 9 2019, 3:36 PM
dschuff accepted D56142: [WebAssembly] Print a debug message at the start of each pass.
Jan 9 2019, 2:43 PM
dschuff added inline comments to D56091: [WebAssembly] Don't add IMPLICIT_DEFs in PrepareForLiveIntervals.
Jan 9 2019, 10:33 AM · Restricted Project

Jan 8 2019

dschuff accepted D56093: [WebAssembly] Rename StoreResults to MemIntrinsicResults.
Jan 8 2019, 11:30 AM

Jan 7 2019

dschuff accepted D56094: [WebAssembly] Fix stack pointer store check in RegStackify.

This patch looks fine with the comment nit. I'm a little unclear because get_global and set_global are still marked as mayLoad and mayStore. Is it a problem with both the explicit modeling of the stack pointer and treating them as reads and writes for this purpose?

Jan 7 2019, 10:28 AM

Dec 19 2018

dschuff added a comment to D52634: [WebAssembly] Add DBG_VALUE with local operands location in WebAssemblyExplicitLocals pass.

Come to think of it, might we actually want the possibility of constant offset in addition to the "kind" and index field, e.g. for cases where the value is actually a pointer?

Dec 19 2018, 10:37 AM · debug-info
dschuff added a comment to D52634: [WebAssembly] Add DBG_VALUE with local operands location in WebAssemblyExplicitLocals pass.

I think there will eventually be more consumers, including traditional-style debuggers (even if just for non-web use cases).
I had initially hoped that we could get away with just using DWARF registers more-or-less directly for locals, but as Yury pointed out, there are a lot of places that assume a fixed number of architectural registers. And of course wasm globals and the wasm value stack don't really have analogs on most architectures. I still hope we'll be able to use DWARF's CFA and other stack stuff for the stack in linear memory. (Although in our case the stack pointer will be a global rather than a register. Hopefully we can make that work.)

Dec 19 2018, 10:28 AM · debug-info

Dec 18 2018

dschuff added a comment to D52634: [WebAssembly] Add DBG_VALUE with local operands location in WebAssemblyExplicitLocals pass.

@aprantl Is the advantage of your suggested approach just that we don't have to define a new expression type? Obviously the interpretation is not the same as DW_OP_breg on other targets so as you say, either way there would have to be special logic in all the tools that consume it. Is this kind of repurposing of builtin primitives common?

Dec 18 2018, 8:43 PM · debug-info

Dec 17 2018

dschuff accepted D55797: [WebAssembly] Make assembler check for proper nesting of control flow..
Dec 17 2018, 5:09 PM

Dec 10 2018

dschuff accepted D55347: [WebAssembly] TargetStreamer cleanup (NFC).

At a high level (i.e. across all the LLVM targets) the intention is that binary and asm streamers have the same interface. That's sort of enforced by the fact that they both have the same base class, but I guess in actuality the "interface" also includes the data on the symbol object. I think I agree that it makes more sense to think of the symbols as owned outside the streamer and have them be a way to pass info into the streamer (the current code is more like having them be state that is only needed by the binary streamer but which lives outside the streamer). I guess that's also another way of saying that symbol type and module name are properties of a symbol even if those properties are only needed by one of the streamers. I guess it might also be useful for debugging or future modifications to have the contents of the symbols be the same regardless of which output type is in use.

Dec 10 2018, 4:06 PM
dschuff added inline comments to D55401: [WebAssembly] Fix assembler parsing of br_table..
Dec 10 2018, 2:29 PM
dschuff accepted D55526: Fix LLVM_LINK_LLVM_DYLIB build of TextAPI.

I don't 100% understand the distinction but if you've tested with LLVM_LINK_LLVM_DYLIB and BUILD_SHARED_LIBS and the static build then LGTM :)

Dec 10 2018, 2:01 PM

Dec 7 2018

dschuff added inline comments to D55401: [WebAssembly] Fix assembler parsing of br_table..
Dec 7 2018, 1:45 PM

Dec 3 2018

dschuff added inline comments to D55149: [WebAssembly] Enforce assembler emits to streamer in order..
Dec 3 2018, 10:10 AM

Nov 28 2018

dschuff added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Nov 28 2018, 10:59 AM

Nov 27 2018

dschuff added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Nov 27 2018, 4:44 PM
dschuff added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Nov 27 2018, 4:40 PM

Nov 19 2018

dschuff added a comment to D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..

Yeah I think for PIC code all globals need to be relative.

Nov 19 2018, 5:41 PM · Restricted Project
dschuff added a comment to D54683: [WebAssembly] Delete unused using statements (NFC).

LLVM doesn't actually have an IWYU guideline though: https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible ... but also not a "dont IWYU".

Nov 19 2018, 5:31 PM
dschuff added inline comments to D54689: [WebAssembly] Rename function types to signatures.
Nov 19 2018, 5:23 PM
dschuff added a comment to D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..

Not sure I am parsing your commit message right. So we fold if DSO local, and not otherwise?

Nov 19 2018, 4:29 PM · Restricted Project
dschuff added inline comments to D54689: [WebAssembly] Rename function types to signatures.
Nov 19 2018, 4:25 PM

Nov 16 2018

dschuff accepted D54652: [WebAssembly] replaced .param/.result by .functype.

otherwise LGTM!

Nov 16 2018, 4:12 PM
dschuff accepted D54644: [WebAssembly] Cleanup unused declares in test code.
Nov 16 2018, 12:39 PM
dschuff accepted D54637: [WebAssembly] Default to static reloc model.
Nov 16 2018, 10:45 AM