Page MenuHomePhabricator

sbc100 (Sam Clegg)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 16 2016, 10:22 AM (117 w, 4 d)

Recent Activity

Fri, Dec 14

sbc100 accepted D54924: [WebAssembly] Check if the section order is correct.
Fri, Dec 14, 4:01 PM
sbc100 added inline comments to D54924: [WebAssembly] Check if the section order is correct.
Fri, Dec 14, 4:00 PM
sbc100 added inline comments to D54924: [WebAssembly] Check if the section order is correct.
Fri, Dec 14, 8:57 AM

Wed, Dec 12

sbc100 created D55613: [WebAssembly] Add "needed" list to dylink section.
Wed, Dec 12, 1:27 PM
sbc100 updated the diff for D55609: [WebAssembly] Update dylink section parsing.
  • update test
Wed, Dec 12, 1:27 PM
sbc100 added reviewers for D55609: [WebAssembly] Update dylink section parsing: jgravelle-google, kripken.
Wed, Dec 12, 12:26 PM
sbc100 created D55609: [WebAssembly] Update dylink section parsing.
Wed, Dec 12, 12:25 PM

Mon, Dec 10

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

If @aardappel and @dschuff are ok with this change I'll defer to them.

Mon, Dec 10, 2:45 PM
sbc100 added a reviewer for D55526: Fix LLVM_LINK_LLVM_DYLIB build of TextAPI: dschuff.
Mon, Dec 10, 1:58 PM
sbc100 updated the summary of D55526: Fix LLVM_LINK_LLVM_DYLIB build of TextAPI.
Mon, Dec 10, 11:35 AM
sbc100 created D55526: Fix LLVM_LINK_LLVM_DYLIB build of TextAPI.
Mon, Dec 10, 11:32 AM

Fri, Dec 7

sbc100 accepted D54875: [WebAssembly] Add support for the event section.
Fri, Dec 7, 10:58 AM
sbc100 accepted D55353: [WebAssembly] Add '.eventtype' directive support.
Fri, Dec 7, 10:56 AM
sbc100 reclaimed D55381: [llvm-tapi] Don't try to override SequenceTraits for std::string.

Reopening as the alternative solution had issues.

Fri, Dec 7, 10:48 AM

Thu, Dec 6

sbc100 accepted D55350: [WebAssembly] clang-format/clang-tidy AsmParser (NFC).
Thu, Dec 6, 5:17 PM
sbc100 added a comment to D54875: [WebAssembly] Add support for the event section.

This change is looking nice!

Thu, Dec 6, 4:32 PM
sbc100 accepted D55390: [llvm-tapi] Don't override SequenceTraits for std::string.

Thanks. lgtm

Thu, Dec 6, 3:23 PM
sbc100 added inline comments to D55350: [WebAssembly] clang-format/clang-tidy AsmParser (NFC).
Thu, Dec 6, 3:09 PM
sbc100 abandoned D55381: [llvm-tapi] Don't try to override SequenceTraits for std::string.
Thu, Dec 6, 12:55 PM
sbc100 added a comment to D55381: [llvm-tapi] Don't try to override SequenceTraits for std::string.

Apologies, I'll keep a closer eye on that in the future. This can be resolve by doing what ELFArch has done. This requires adding a typedef to TBEHandler.h, changing the declaration of NeededLibs to use the new typedef, and then modifying the ELFYAMLTest.cpp to use something like

LLVM_YAML_STRONG_TYPEDEF(ELFNeededList, ELFNeededMapper)

That should allow us to keep the flow mapping.

Thu, Dec 6, 12:08 PM
sbc100 added inline comments to D55347: [WebAssembly] TargetStreamer cleanup (NFC).
Thu, Dec 6, 12:06 PM
sbc100 added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

Attempted fix in https://reviews.llvm.org/D55381

Thu, Dec 6, 11:42 AM
sbc100 added a reviewer for D55381: [llvm-tapi] Don't try to override SequenceTraits for std::string: amontanez.
Thu, Dec 6, 11:41 AM
sbc100 retitled D55381: [llvm-tapi] Don't try to override SequenceTraits for std::string from Don't try to override SequenceTraits for std::string to [llvm-tapi] Don't try to override SequenceTraits for std::string.
Thu, Dec 6, 11:41 AM
sbc100 created D55381: [llvm-tapi] Don't try to override SequenceTraits for std::string.
Thu, Dec 6, 11:41 AM
sbc100 added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

We are seeing a failure of YAMLWritesNoTBESyms on the WebAssembly waterfall due to this change:

Thu, Dec 6, 11:18 AM
sbc100 accepted D55353: [WebAssembly] Add '.eventtype' directive support.

lgtm % comment

Thu, Dec 6, 8:39 AM
sbc100 added a comment to D55353: [WebAssembly] Add '.eventtype' directive support.

Since event types can't have return values (they are always no_return right?) should we split out "parseParamList" and have them look like this?

Thu, Dec 6, 8:39 AM
sbc100 added a comment to D55347: [WebAssembly] TargetStreamer cleanup (NFC).

There is a presensent for the "emit" functions in the streamer modifying symbols. For example:

Thu, Dec 6, 8:35 AM
sbc100 added inline comments to D55347: [WebAssembly] TargetStreamer cleanup (NFC).
Thu, Dec 6, 8:31 AM
sbc100 accepted D55350: [WebAssembly] clang-format/clang-tidy AsmParser (NFC).
Thu, Dec 6, 8:27 AM

Wed, Dec 5

sbc100 accepted D55343: [WebAssembly] Change event section code to 13.
Wed, Dec 5, 3:11 PM

Tue, Dec 4

sbc100 accepted D55247: [WebAssembly] Make WasmSymbol's signature usable for events (NFC).

IIUC this change is NFC so you can mark it as such in the title?

Tue, Dec 4, 10:19 PM
sbc100 added inline comments to D55247: [WebAssembly] Make WasmSymbol's signature usable for events (NFC).
Tue, Dec 4, 9:45 AM

Mon, Dec 3

sbc100 accepted D55247: [WebAssembly] Make WasmSymbol's signature usable for events (NFC).

Should the first word of the description be WasmSymbol

Mon, Dec 3, 9:18 PM
sbc100 added a comment to D54873: [WebAssembly] Make signature index not a part of EventType.

Yes in that case it's represented like sigindex of functions.

- Type:            FUNCTION                                                    
  FunctionTypes:   [ 0, 0 ]                                                    
- Type:            EVENT                                                       
  EventTypes:      [ 1 ]                   // here                                        
  Events:                                                 
    - Index:           0                                                       
      Attribute:       0
Mon, Dec 3, 3:02 PM
sbc100 added a comment to D54873: [WebAssembly] Make signature index not a part of EventType.

And why do we need this for specifying events themselves and for importing events?

I mean event imports need to specify their "type" and non-imported events also need to specify their "type". And in both cases the type needs to include the signature. So I think we should have a unified "eventType" that includes the signature index.

I can drop this CL, but I still don't understand what you meant by this part. Currently imported exception's sigindex is handled fine. The below is obj2yaml's result if I don't define the __cpp_exception symbol:

- Type:            IMPORT                                                      
  Imports:                                                                     
    ...                                    
    - Module:          env                                                     
      Field:           __cpp_exception                                         
      Kind:            EVENT                                                   
      SigIndex:        1                                                       
      EventAttribute:  0

As you see in WasmImport definition in this CL, SigIndex is now out of union and every type can set it optionally, so there's no problem for events having sigindex there.

struct WasmImport {                                                              
  StringRef Module;                                                              
  StringRef Field;                                                               
  uint8_t Kind;                                                                  
  uint32_t SigIndex;                                                             
  union {                                                                        
    WasmGlobalType Global;                                                       
    WasmTable Table;                                                             
    WasmLimits Memory;                                                           
    WasmEventType Event;                                                         
  };                                                                             
};
Mon, Dec 3, 2:52 PM
sbc100 added reviewers for D55231: [WebAssembly] Don't set a maximum side when importing the table: ruiu, jgravelle-google.
Mon, Dec 3, 12:28 PM
sbc100 created D55231: [WebAssembly] Don't set a maximum side when importing the table.
Mon, Dec 3, 12:27 PM
sbc100 accepted D55149: [WebAssembly] Enforce assembler emits to streamer in order..
Mon, Dec 3, 12:17 PM
sbc100 added inline comments to D55149: [WebAssembly] Enforce assembler emits to streamer in order..
Mon, Dec 3, 9:39 AM
sbc100 added a comment to D54873: [WebAssembly] Make signature index not a part of EventType.

Ping. So the decision points are

  1. Whether SigIndex should be in or out of WasmEventType?
    1. If SigIndex should be out of WasmEventType, should there even be WasmEventType anymore? Because then there's only Attribute left.
  2. It seems to be people don't like the term Attribute. Should we change it to Kind or something? We can change the spec text, because it's not set in stone or anything.

Seems like we should keep the two member WasmEventType. We need this for specifying the events themselves, and also for importing events. It seems like this matches WasmGlobalType,

I'm not really sure what this means. Do you mean we should keep SigIndex as a part of WasmEventType and drop this CL?

Mon, Dec 3, 9:38 AM

Fri, Nov 30

sbc100 added a comment to D54873: [WebAssembly] Make signature index not a part of EventType.

Ping. So the decision points are

  1. Whether SigIndex should be in or out of WasmEventType?
    1. If SigIndex should be out of WasmEventType, should there even be WasmEventType anymore? Because then there's only Attribute left.
  2. It seems to be people don't like the term Attribute. Should we change it to Kind or something? We can change the spec text, because it's not set in stone or anything.
Fri, Nov 30, 2:49 PM

Wed, Nov 28

sbc100 added a reviewer for D55043: [WebAssembly] Allow undefined symbols when building shared libraries: ruiu.
Wed, Nov 28, 7:07 PM
sbc100 created D55043: [WebAssembly] Allow undefined symbols when building shared libraries.
Wed, Nov 28, 7:07 PM
sbc100 updated the diff for D54982: [WebAssembly] Update docs.
  • feedback
Wed, Nov 28, 4:12 PM
sbc100 added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Wed, Nov 28, 1:31 PM
sbc100 added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Wed, Nov 28, 11:09 AM
sbc100 added a comment to D54924: [WebAssembly] Check if the section order is correct.

D44024 is not simpler; actually it is more complicated because it checks more things than this, such as custom sections and whether the function section matches with the code section. If you think we should also include custom section checking in this CL, why don't we just land D44024?

Wed, Nov 28, 8:49 AM
sbc100 added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Wed, Nov 28, 8:42 AM

Tue, Nov 27

sbc100 added a comment to D54924: [WebAssembly] Check if the section order is correct.

I see.. so why wasn't it landed? It says it was stalled because we couldn't decide whether or how to check custom section orderings and orderings between custom section vs. normal sections. Do we now have consensus on that? If so landing that CL makes more sense. If not, do you think now is a good time to re-discuss it? Or would landing this CL first and decide what to do with custom sections later makes sense?

The main motivation for this CL was, I had to delete code that was previously checking relative section orders in yaml2wasm because the previous code only checked if the section codes were monotonically increasing and the new event section had code 12, and you suggested we should reinstate it in a separate CL.

Tue, Nov 27, 5:39 PM
sbc100 added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Tue, Nov 27, 4:26 PM
sbc100 added a reviewer for D54982: [WebAssembly] Update docs: ruiu.
Tue, Nov 27, 3:48 PM
sbc100 created D54982: [WebAssembly] Update docs.
Tue, Nov 27, 3:47 PM
sbc100 added inline comments to D54875: [WebAssembly] Add support for the event section.
Tue, Nov 27, 3:14 PM
sbc100 updated subscribers of D54875: [WebAssembly] Add support for the event section.

lgtm % nits and the question of whether we leave SigIndex as part of the EventType in wasm.h.

Tue, Nov 27, 3:11 PM
sbc100 added inline comments to D54873: [WebAssembly] Make signature index not a part of EventType.
Tue, Nov 27, 2:52 PM
sbc100 added a comment to D54924: [WebAssembly] Check if the section order is correct.

For reference there was a previous CL that tries to do something similar: https://reviews.llvm.org/D44024.

Tue, Nov 27, 9:25 AM

Mon, Nov 26

sbc100 updated the diff for D54758: [WebAssembly] Remove `using` statements from header files. NFC..
  • sort
Mon, Nov 26, 3:55 PM
sbc100 updated the diff for D54758: [WebAssembly] Remove `using` statements from header files. NFC..
  • feedback
Mon, Nov 26, 3:52 PM
sbc100 added a comment to D54758: [WebAssembly] Remove `using` statements from header files. NFC..

I think some redundant includes should definitely go (IWYU). Regarding more granular using declarations, honestly it's up to @ruiu, Clang and LLDB handle it by aliasing some of LLVM's common classes in their respective namespaces, maybe that could work for LLD too? In fact I'm guessing that's the purpose of lld/Common/LLVM.h, you could take a similar approach for Wasm specific stuff and limit the using statements to your namespace.

Your guess is correct; lld/Common/LLVM.h is to using some identifiers so that they can be referred without namespace specifiers. In fact, I think it is fine to add Wasm* names to that file because it is obvious that they will never conflict with other names because of Wasm prefix. Some names are ambiguous, such as llvm::object::Archive, and it is better to refer them using a fully qualified name, as we have other classes with confusing names (e.g. lld::*::ArchiveFile).

Mon, Nov 26, 3:37 PM
sbc100 added inline comments to D54647: WIP: [WebAssembly] First pass the implement -fPIC.
Mon, Nov 26, 10:17 AM

Tue, Nov 20

sbc100 added a comment to D54689: [WebAssembly] Rename function types to signatures.

I general I think this is good change. I think there will always be point in the abstraction where we start prefering "spec" terms over "llvm" terms.

Tue, Nov 20, 5:40 PM
sbc100 added a comment to D54758: [WebAssembly] Remove `using` statements from header files. NFC..

I think what @dschuff said in this comment was not removing all using from headers, but removing using namespace from headers.. no? We are not using using namespace in header files already.

Tue, Nov 20, 4:48 PM
sbc100 added reviewers for D54758: [WebAssembly] Remove `using` statements from header files. NFC.: ruiu, dschuff, aheejin.
Tue, Nov 20, 8:22 AM
sbc100 created D54758: [WebAssembly] Remove `using` statements from header files. NFC..
Tue, Nov 20, 8:22 AM
sbc100 accepted 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".

Also this general problem is why Chrome has a rule against using namespace in header files. I like that rule because you can always tell by looking in the cpp file which namespaces are in scope.
If we do the same, then using directive in a cpp file can be removed if the file still compiles after removing it because we know it's not coming from somewhere else.

Also even if we adopt that convention for the wasm backend we might have to make an exception for using namespace llvm because it's so ubiquitous.

Tue, Nov 20, 7:28 AM

Mon, Nov 19

sbc100 accepted D54734: [WebAssembly] Remove unused function return types (NFC).
Mon, Nov 19, 4:37 PM
sbc100 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?

Mon, Nov 19, 4:36 PM
sbc100 updated the summary of D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..
Mon, Nov 19, 4:34 PM
sbc100 added a comment to D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..

Yes, this should have no effect unless we are compiling with -fPIC and have non-DSO-local symbols.

Mon, Nov 19, 4:33 PM
sbc100 added inline comments to D54690: [WebAssembly] Rename function types to signatures.
Mon, Nov 19, 12:41 PM
sbc100 accepted D54689: [WebAssembly] Rename function types to signatures.

lgtm %nits

Mon, Nov 19, 12:38 PM
sbc100 accepted D54688: [WebAssembly] Fix inaccurate comments / assertion messages.
Mon, Nov 19, 12:31 PM
sbc100 accepted D54687: [WebAssembly] Make starting indices calcaulation simpler (NFC).

Hmm, I guess at some point in the past InputFunctions and InputGlobals were not empty at the start of this function.

Mon, Nov 19, 12:30 PM
sbc100 added a comment to D54683: [WebAssembly] Delete unused using statements (NFC).

I'm not sure how you created this change, but I think using statements are like IWYU in that you don't want remove any line that can be removed and have the file still compile. i.e. you don't want to rely on recursively included using statements.

Mon, Nov 19, 11:45 AM

Nov 17 2018

sbc100 accepted D54662: [WebAssembly] Add equality comparison operators for WasmEventType.
Nov 17 2018, 10:03 AM

Nov 16 2018

sbc100 retitled D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC. from [WebAssembly] Don't override default implementation of isOffsetFoldingLegal to [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..
Nov 16 2018, 5:00 PM
sbc100 created D54661: [WebAssembly] Don't override default implementation of isOffsetFoldingLegal. NFC..
Nov 16 2018, 4:59 PM
sbc100 updated the diff for D54647: WIP: [WebAssembly] First pass the implement -fPIC.

get parts of address-offsets.ll passing

Nov 16 2018, 4:36 PM
sbc100 accepted D54652: [WebAssembly] replaced .param/.result by .functype.

Nice! I wish we could do with without the lexer change and with a more asm-linux syntax.. but I guess that time for bikeshedding that is over.

Nov 16 2018, 2:56 PM
sbc100 accepted D54614: [WebAssembly] Fix MCNullStreamer support.
Nov 16 2018, 2:49 PM
sbc100 added a comment to D54647: WIP: [WebAssembly] First pass the implement -fPIC.

@dschuff can you take a look at the WebAssemblyFastISel.cpp part? I want to make sure I'm on the right track.

Nov 16 2018, 1:50 PM
sbc100 updated the summary of D54647: WIP: [WebAssembly] First pass the implement -fPIC.
Nov 16 2018, 1:48 PM
sbc100 created D54647: WIP: [WebAssembly] First pass the implement -fPIC.
Nov 16 2018, 1:28 PM
sbc100 added a reviewer for D54644: [WebAssembly] Cleanup unused declares in test code: dschuff.
Nov 16 2018, 12:28 PM
sbc100 updated the diff for D54644: [WebAssembly] Cleanup unused declares in test code.

cleanup

Nov 16 2018, 12:26 PM
sbc100 created D54644: [WebAssembly] Cleanup unused declares in test code.
Nov 16 2018, 12:13 PM
sbc100 added a reviewer for D54637: [WebAssembly] Default to static reloc model: dschuff.
Nov 16 2018, 10:32 AM
sbc100 created D54637: [WebAssembly] Default to static reloc model.
Nov 16 2018, 10:31 AM
sbc100 added inline comments to D54614: [WebAssembly] Fix MCNullStreamer support.
Nov 16 2018, 9:02 AM

Nov 15 2018

sbc100 accepted D54586: [WebAssembly] Fix return type of nextByte.
Nov 15 2018, 10:43 AM
sbc100 accepted D54572: [WebAssembly] Change type of wake count to unsigned int.
Nov 15 2018, 10:28 AM
sbc100 updated subscribers of D54571: [WebAssembly] Split BBs after throw instructions.

Sorry for little offtopic at first.

WebAssembly folks, can you check this warning:

WebAssemblyDisassembler.cpp:125:11: warning: comparison is always false due to limited range of data type [-Wtype-limits]

if (Opc < 0)

GCC 8.2

Nov 15 2018, 10:27 AM
sbc100 updated the diff for D54558: [WebAssembly] Import the stack pointer when building shared libraries.
  • feedback
  • clang-format
Nov 15 2018, 10:18 AM

Nov 14 2018

sbc100 added a reviewer for D54559: [WebAssembly] Refactor config setting and checking. NFC.: ruiu.
Nov 14 2018, 5:20 PM
sbc100 added a comment to D54559: [WebAssembly] Refactor config setting and checking. NFC..

The diff algorithms seem to make a right mess of this, but this change basically splits out setConfigs and checkOptions and nothing more

Nov 14 2018, 5:20 PM
sbc100 created D54559: [WebAssembly] Refactor config setting and checking. NFC..
Nov 14 2018, 5:18 PM
sbc100 added reviewers for D54558: [WebAssembly] Import the stack pointer when building shared libraries: dschuff, ruiu.
Nov 14 2018, 5:12 PM
sbc100 created D54558: [WebAssembly] Import the stack pointer when building shared libraries.
Nov 14 2018, 5:10 PM