Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1837663002:
Initial Subzero WASM prototype. (Closed)
|
Created:
4 years, 9 months ago by Eric Holk Modified:
4 years, 8 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionInitial Subzero WASM prototype.
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=16f8061ca23653172ea4c1a43659bb0501b5b474
Patch Set 1 #Patch Set 2 : Removing torture tests. #Patch Set 3 : Updating Makefile.standalong to disable WASM by default. #Patch Set 4 : Cleanup #
Total comments: 182
Patch Set 5 : Review feedback from JF and John #Patch Set 6 : Minor formatting change #Patch Set 7 : Use tables instead of extra fields in Ice data structures #Patch Set 8 : Remove defining instruction field from IceOperand #Patch Set 9 : Incorporating Jim's suggestions #Patch Set 10 : Merging with master #
Total comments: 80
Patch Set 11 : Review feedback from Jim #Patch Set 12 : Merging master #Patch Set 13 : Make presubmit great again #
Total comments: 37
Patch Set 14 : Incorporating review feedback #
Total comments: 9
Patch Set 15 : Code review feedback and merging master #
Messages
Total messages: 16 (3 generated)
eholk@chromium.org changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, sehr@chromium.org, stichnot@chromium.org
jfb@chromium.org changed reviewers: + jfb@chromium.org
A few drive-bys. https://codereview.chromium.org/1837663002/diff/40017/README-wasm.md File README-wasm.md (right): https://codereview.chromium.org/1837663002/diff/40017/README-wasm.md#newcode6 README-wasm.md:6: LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target make -j48 -f Makefile.standalone WASM=1 && LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target ./pnacl-sz -O2 -filetype=asm -target=arm32 ./torture-s2wasm-sexpr-wasm/20000112-1.c.s.wast.wasm Link to wasm-stat.us to download torture tests. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp#newc... src/IceCfgNode.cpp:63: assert(false); llvm_unreachable https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:32: #endif Why not put the #if in the other header? https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:64: std::regex_match(Filename, std::regex(".*\\.wasm")); Eek, I wouldn't use regex for this. It's a huge hammer. StringRef has endswith(). https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:111: #endif Would be nice if this switch were only visible from the WasmTranslator.h file: WasmTranslator could just be a stub class which emits this error. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:1: #include "WasmTranslator.h" Copyright. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:49: assert(false && "kTagged type not supported"); llvm_unreachable for the few asserts above. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:681: uint8_t Buffer[BUFFER_SIZE]; Can you allocate this instead? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h File src/WasmTranslator.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:1: #pragma once Copyright notice for the file. Also, pragma once? Include guards are standard. https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.py File wasm-run-torture-tests.py (right): https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.... wasm-run-torture-tests.py:1: #!/usr/bin/python3 I think the following is more common: #!/usr/bin/env python3 Also, copyright.
style-related comments for now. I'll come back to this later today. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:54: This class wraps either and Operand or a CfgNode. an Operand https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:72: operator Operand *() const { explicit? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:77: return (Operand *)Data; reinterpret_cast<Operand *>(Data); https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:88: operator bool() const { return (Data != UNDEF_PTR) && Data; } explicit operator? -- I personally do not like implicit boolean casts. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:89: bool operator!=(const OperandNode &Rhs) const { why not: bool operator==() { return Data == Rhs.Data; } bool operator!=() { return *this != *that; } would it work? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:101: auto Oper = (Operand *)Op; static_cast -- or even better, Op.getOperand(); https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:134: #define LOG(Expr) log([&](Ostream & out) { Expr; }) default capture by reference -- even on a define -- ... :( https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:138: Move the following to the end of the class declaration. Don't forget to delete CopyCtor/CopyOper https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:139: wasm::ModuleEnv *module_; Module https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:150: Fn(Ctx->getStrDump()); don't you want to lock the ostream before invoking Fn? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:157: : Cfg(Cfg), Ctx(Cfg->getContext()), control_(nullptr) {} I would love member to be named all lower case, with a trailing underscore -- but while we stick with llvm's dumb convetion, please s/control_/Control/g https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:159: Node *Buffer(size_t Count) { allocateBuffer -- the name Buffer() does not really tell me what this does ... or, if it maps to a wasm construct, comment what it is -- ditto everywhere. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:167: auto Entry = Cfg->makeNode(); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:174: auto Arg = Cfg->makeVariable(toIceType(Type)); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:183: Node Loop = Cfg->makeNode(); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:200: auto MergedNode = Cfg->makeNode(); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:216: const auto InEdges = ((CfgNode *)Control)->getInEdges(); static_cast<> https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:221: auto Dest = Cfg->makeVariable(((Operand *)Vals[0])->getType()); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:227: auto Phi = InstPhi::create(Cfg, Count * 10, Dest); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:229: auto Op = (Operand *)Vals[i]; static_cast<> https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:233: ((CfgNode *)Control)->appendInst(Phi); static_cast<> https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:240: for (auto i = 0; i < Count; ++i) { no auto for integral types?... https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:247: auto Const = Ctx->getConstantInt32(Value); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:256: auto Const = Ctx->getConstantInt64(Value); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:264: auto Const = Ctx->getConstantFloat(Value); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:272: auto Const = Ctx->getConstantDouble(Value); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:281: Ice::Variable *Dest = Cfg->makeVariable( auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:282: isComparison(Opcode) ? IceType_i1 : ((Operand *)Left)->getType()); static_cast<> https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:386: auto Zero = Cfg->makeVariable(IceType_f32); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:387: Control()->appendInst(InstAssign::create(Cfg, Zero, Ctx->getConstantFloat(0))); 80 col https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:388: Control()->appendInst(InstArithmetic::create(Cfg, InstArithmetic::Fsub, Dest, Zero, Input)); 80 col https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:393: auto Zero = Cfg->makeVariable(IceType_f64); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:394: Control()->appendInst(InstAssign::create(Cfg, Zero, Ctx->getConstantDouble(0))); 80 col https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:395: Control()->appendInst(InstArithmetic::create(Cfg, InstArithmetic::Fsub, Dest, Zero, Input)); 80 col https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:410: unsigned InputCount(CfgNode *Node) { return Node->getInEdges().size(); } this should not be a member class -- it is really not part of the builder... it should, at lease, be static. we tend to favor using the integer types in <cstdint> (i.e., uint32_t instead of unsigned.) https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:417: if (auto RealPhi = llvm::dyn_cast<Ice::Variable>((Operand *)Phi)) { auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:420: if (auto Inst = RealPhi->getDefiningInst()) { auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:436: void AppendToMerge(CfgNode *Merge, CfgNode *From) { AppendToMerge is a funny name; you're not really appending stuff... can this method be marked const? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:442: auto Var = llvm::cast<Ice::Variable>((Operand *)Phi); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:444: auto Inst = llvm::cast<InstPhi>(Var->getDefiningInst()); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:445: auto FromVar = llvm::cast<Ice::Variable>((Operand *)From); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:452: nullptr_t Branch(Node Cond, Node *TrueNode, Node *FalseNode) { void instead? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:457: auto Ctrl = Control(); auto *? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:478: auto Instr = auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:487: auto Instr = InstRet::create(Cfg); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:495: auto Instr = InstUnreachable::create(Cfg); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:508: const auto Sig = Target.sig; auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:513: const auto TargetName = Module->GetName(Target.name_offset); std::string? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:518: auto TargetOperand = Ctx->getConstantSym(0, TargetName); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:520: auto Dest = Sig->return_count() > 0 auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:523: auto Call = InstCall::create(Cfg, NumArgs, Dest, TargetOperand, auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:541: const auto Sig = module_->GetImportSignature(Index); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:552: auto TargetOperand = Ctx->getConstantSym(0, TargetName); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:554: auto Dest = Sig->return_count() > 0 auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:557: auto Call = InstCall::create(Cfg, NumArgs, Dest, TargetOperand, auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:590: auto OffsetConstant = Ctx->getConstantInt32(Offset); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:591: auto Addr = Cfg->makeVariable(IceType_i32); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:596: auto LoadResult = Cfg->makeVariable(toIceType(MemType)); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:620: auto OffsetConstant = Ctx->getConstantInt32(Offset); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:621: auto Addr = Cfg->makeVariable(IceType_i32); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:628: auto LocalStoreVal = Cfg->makeVariable(toIceType(Type)); auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:643: return control_ ? (CfgNode *)*control_ : Cfg->getEntryNode(); static_cast<> https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:647: void set_module(wasm::ModuleEnv *Module) { this->module_ = Module; } setModule https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:649: void set_control_ptr(Node *Control) { this->control_ = Control; } setControlPtr https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:651: void set_effect_ptr(Node *Effect) { this->effect_ = Effect; } setEffectPtr https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:691: auto BytesRead = InputStream->GetBytes(Buffer, sizeof(Buffer)); integral type? don't use auto? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:740: auto NewName = fnNameFromId(Id++); std::string? https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:751: auto Cfg = auto * https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h File src/WasmTranslator.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:8: namespace v8::internal { we tend to do namespace v8 { namespace internal { https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:8: namespace v8::internal { I would just #include the files defining these... https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:9: class Zone; no spaces? make sure clang-format is happy here. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:12: } // end of namespace wasm https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:13: } // end of namespace v8::internal https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:34: std::unique_ptr<llvm::DataStreamer> &&InputStream); std::unique_ptr<> InputStream (i.e., no r-value reference) https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:42: } // end of namespace Ice
Initial comments. I'll make comments on the new parsing files after John's comments are incorporated. https://codereview.chromium.org/1837663002/diff/40017/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/40017/Makefile.standalone#new... Makefile.standalone:273: CXXFLAGS += $(V8_CXXFLAGS) -DSUBZERO_WASM Follow the pattern above where you also modify OBJDIR. Also, I suggest -DALLOW_WASM to follow the other ALLOW_* defines. https://codereview.chromium.org/1837663002/diff/40017/Makefile.standalone#new... Makefile.standalone:425: $(V8_LIBS) untabify this line (or just add it to the previous line) https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.cpp#newcode235 src/IceCfg.cpp:235: const auto InEdges = Node->getInEdges(); Should this be "const auto &" ? I don't know. getInEdges() returns a ref, and I'm not sure whether you're accidentally making a copy here. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.cpp#newcode237 src/IceCfg.cpp:237: auto Phi = llvm::cast<InstPhi>(&Inst); auto * https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.h#newcode268 src/IceCfg.h:268: /// The WASM translater cannot always determine the right incoming edge for a translator https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp#newc... src/IceCfgNode.cpp:45: if (auto Dest = Instr->getDest()) { auto * here and below https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp#newc... src/IceCfgNode.cpp:63: assert(false); On 2016/03/28 18:17:13, JF wrote: > llvm_unreachable actually just remove the assert. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.h File src/IceCfgNode.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.h#newcod... src/IceCfgNode.h:113: void addOutEdge(CfgNode *out) { OutEdges.push_back(out); } Capitalize Out and In vars per LLVM convention. https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:100: #ifdef SUBZERO_WASM We try to minimize the use of #ifdef code. Where possible, the code is conditional on some Ice::BuildDefs constexpr function so that mistakes are caught by the compiler in early development. Would that be possible here? https://codereview.chromium.org/1837663002/diff/40017/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceOperand.h#newcod... src/IceOperand.h:785: Inst *DefiningInst = nullptr; I'm not super excited about increasing the size of operands/variable/instructions for what is currently wasm-specific info. Also, I don't see that this info is being updated when: 1) the defining instruction changes e.g. during lowering when the original defining instruction is deleted and a new one is inserted 2) the defining node changes due to node splitting I think a more appropriate approach would be to maintain various unordered_maps during parsing and then discard them after all is parsed. https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.py File wasm-run-torture-tests.py (right): https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.... wasm-run-torture-tests.py:15: cmd = "LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target ./pnacl-sz -filetype=asm -target=arm32 {} -threads=0 -O2 -verbose=wasm".format(test_file) 80-col Yucky absolute paths. -target=arm32??? Move this file into pydir/ so that it can later benefit from other utilities there.
I've incorporated the feedback from JF and John, and a little bit from Jim. https://codereview.chromium.org/1837663002/diff/40017/README-wasm.md File README-wasm.md (right): https://codereview.chromium.org/1837663002/diff/40017/README-wasm.md#newcode6 README-wasm.md:6: LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target make -j48 -f Makefile.standalone WASM=1 && LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target ./pnacl-sz -O2 -filetype=asm -target=arm32 ./torture-s2wasm-sexpr-wasm/20000112-1.c.s.wast.wasm On 2016/03/28 18:17:13, JF wrote: > Link to wasm-stat.us to download torture tests. Done. A subsequent CL will include a script to download the torture tests from the right build. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp#newc... src/IceCfgNode.cpp:63: assert(false); On 2016/03/28 18:17:13, JF wrote: > llvm_unreachable Actually, I didn't mean to keep this assert here anyway, so I just removed it. Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:32: #endif On 2016/03/28 18:17:13, JF wrote: > Why not put the #if in the other header? Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:64: std::regex_match(Filename, std::regex(".*\\.wasm")); On 2016/03/28 18:17:13, JF wrote: > Eek, I wouldn't use regex for this. It's a huge hammer. StringRef has > endswith(). I just cargo-culted from the previous llvmIRInput function, although I agree it's a bigger hammer than we need. https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:111: #endif On 2016/03/28 18:17:13, JF wrote: > Would be nice if this switch were only visible from the WasmTranslator.h file: > WasmTranslator could just be a stub class which emits this error. Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:1: #include "WasmTranslator.h" On 2016/03/28 18:17:13, JF wrote: > Copyright. Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:49: assert(false && "kTagged type not supported"); On 2016/03/28 18:17:13, JF wrote: > llvm_unreachable for the few asserts above. Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:54: This class wraps either and Operand or a CfgNode. On 2016/03/28 18:44:09, John wrote: > an Operand Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:72: operator Operand *() const { On 2016/03/28 18:44:07, John wrote: > explicit? I'd rather not. The goal was to make something that works seamlessly as either an Operand or a CfgNode. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:77: return (Operand *)Data; On 2016/03/28 18:44:07, John wrote: > reinterpret_cast<Operand *>(Data); Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:88: operator bool() const { return (Data != UNDEF_PTR) && Data; } On 2016/03/28 18:44:10, John wrote: > explicit operator? -- I personally do not like implicit boolean casts. Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:89: bool operator!=(const OperandNode &Rhs) const { On 2016/03/28 18:44:07, John wrote: > why not: > > bool operator==() { return Data == Rhs.Data; } > bool operator!=() { return *this != *that; } > > would it work? It's probably cleaner to have operator== and then negate it for !=, but the logic is more complicated than just `Data == Rhs.Data`. The complication is that this class can represent nullptr in three different ways: UNDEF_PTR, 0 (for an Operand * nullptr) and 1 (for a Node * nullptr). I want UNDEF_PTR to be considered equal to both Operand-null and Node-null, but Operand-null should not be equal to Node-null, because the types are different. Come to think of it, I don't think the logic that was in the code is what I described. I rewrote it with a separate operator== to hopefully make the intent more clear. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:134: #define LOG(Expr) log([&](Ostream & out) { Expr; }) On 2016/03/28 18:44:10, John wrote: > default capture by reference -- even on a define -- ... :( Yeah... I don't super love this either. I don't really like the idea of duplicating the check to see whether logging should be enabled everywhere either though. Surely there's a better way to do this though. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:138: On 2016/03/28 18:44:10, John wrote: > Move the following to the end of the class declaration. > > > Don't forget to delete > > CopyCtor/CopyOper Done and Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:150: Fn(Ctx->getStrDump()); On 2016/03/28 18:44:09, John wrote: > don't you want to lock the ostream before invoking Fn? Done, and also in WasmTranslator.h https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:157: : Cfg(Cfg), Ctx(Cfg->getContext()), control_(nullptr) {} On 2016/03/28 18:44:08, John wrote: > I would love member to be named all lower case, with a trailing underscore -- > but while we stick with llvm's dumb convetion, please > > s/control_/Control/g Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:159: Node *Buffer(size_t Count) { On 2016/03/28 18:44:08, John wrote: > allocateBuffer -- the name Buffer() does not really tell me what this does ... > > or, if it maps to a wasm construct, comment what it is -- ditto everywhere. This name was dictated by the WasmBuilder from V8. I'll add a comment though. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:167: auto Entry = Cfg->makeNode(); On 2016/03/28 18:44:09, John wrote: > auto * I left the * off because I remember your pointer rewriting change was harder because of the explicit pointers everywhere. I'll add them back in though. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:174: auto Arg = Cfg->makeVariable(toIceType(Type)); On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:183: Node Loop = Cfg->makeNode(); On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:200: auto MergedNode = Cfg->makeNode(); On 2016/03/28 18:44:07, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:216: const auto InEdges = ((CfgNode *)Control)->getInEdges(); On 2016/03/28 18:44:10, John wrote: > static_cast<> Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:221: auto Dest = Cfg->makeVariable(((Operand *)Vals[0])->getType()); On 2016/03/28 18:44:08, John wrote: > auto * Done. I also added a static_cast. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:227: auto Phi = InstPhi::create(Cfg, Count * 10, Dest); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:229: auto Op = (Operand *)Vals[i]; On 2016/03/28 18:44:08, John wrote: > static_cast<> Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:233: ((CfgNode *)Control)->appendInst(Phi); On 2016/03/28 18:44:08, John wrote: > static_cast<> Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:240: for (auto i = 0; i < Count; ++i) { On 2016/03/28 18:44:09, John wrote: > no auto for integral types?... Ok. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:247: auto Const = Ctx->getConstantInt32(Value); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:256: auto Const = Ctx->getConstantInt64(Value); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:264: auto Const = Ctx->getConstantFloat(Value); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:272: auto Const = Ctx->getConstantDouble(Value); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:281: Ice::Variable *Dest = Cfg->makeVariable( On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:282: isComparison(Opcode) ? IceType_i1 : ((Operand *)Left)->getType()); On 2016/03/28 18:44:08, John wrote: > static_cast<> Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:387: Control()->appendInst(InstAssign::create(Cfg, Zero, Ctx->getConstantFloat(0))); On 2016/03/28 18:44:08, John wrote: > 80 col Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:388: Control()->appendInst(InstArithmetic::create(Cfg, InstArithmetic::Fsub, Dest, Zero, Input)); On 2016/03/28 18:44:08, John wrote: > 80 col Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:393: auto Zero = Cfg->makeVariable(IceType_f64); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:394: Control()->appendInst(InstAssign::create(Cfg, Zero, Ctx->getConstantDouble(0))); On 2016/03/28 18:44:09, John wrote: > 80 col Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:395: Control()->appendInst(InstArithmetic::create(Cfg, InstArithmetic::Fsub, Dest, Zero, Input)); On 2016/03/28 18:44:08, John wrote: > 80 col Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:410: unsigned InputCount(CfgNode *Node) { return Node->getInEdges().size(); } On 2016/03/28 18:44:07, John wrote: > this should not be a member class -- it is really not part of the builder... it > should, at lease, be static. > > we tend to favor using the integer types in <cstdint> (i.e., uint32_t instead of > unsigned.) This is a function called from V8, so we have to match what they expect there. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:417: if (auto RealPhi = llvm::dyn_cast<Ice::Variable>((Operand *)Phi)) { On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:420: if (auto Inst = RealPhi->getDefiningInst()) { On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:436: void AppendToMerge(CfgNode *Merge, CfgNode *From) { On 2016/03/28 18:44:09, John wrote: > AppendToMerge is a funny name; you're not really appending stuff... > > can this method be marked const? It's from V8 again. The idea is there is a merge node and the decoder discovered a new incoming edge, so it has to append it. I marked it as const and it worked, so I guess that seems alright. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:442: auto Var = llvm::cast<Ice::Variable>((Operand *)Phi); On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:444: auto Inst = llvm::cast<InstPhi>(Var->getDefiningInst()); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:445: auto FromVar = llvm::cast<Ice::Variable>((Operand *)From); On 2016/03/28 18:44:07, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:452: nullptr_t Branch(Node Cond, Node *TrueNode, Node *FalseNode) { On 2016/03/28 18:44:08, John wrote: > void instead? I tried, but V8 expects this function to return a value. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:457: auto Ctrl = Control(); On 2016/03/28 18:44:07, John wrote: > auto *? Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:478: auto Instr = On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:487: auto Instr = InstRet::create(Cfg); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:495: auto Instr = InstUnreachable::create(Cfg); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:508: const auto Sig = Target.sig; On 2016/03/28 18:44:07, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:513: const auto TargetName = Module->GetName(Target.name_offset); On 2016/03/28 18:44:07, John wrote: > std::string? Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:523: auto Call = InstCall::create(Cfg, NumArgs, Dest, TargetOperand, On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:541: const auto Sig = module_->GetImportSignature(Index); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:552: auto TargetOperand = Ctx->getConstantSym(0, TargetName); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:554: auto Dest = Sig->return_count() > 0 On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:557: auto Call = InstCall::create(Cfg, NumArgs, Dest, TargetOperand, On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:590: auto OffsetConstant = Ctx->getConstantInt32(Offset); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:591: auto Addr = Cfg->makeVariable(IceType_i32); On 2016/03/28 18:44:08, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:596: auto LoadResult = Cfg->makeVariable(toIceType(MemType)); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:620: auto OffsetConstant = Ctx->getConstantInt32(Offset); On 2016/03/28 18:44:07, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:621: auto Addr = Cfg->makeVariable(IceType_i32); On 2016/03/28 18:44:10, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:628: auto LocalStoreVal = Cfg->makeVariable(toIceType(Type)); On 2016/03/28 18:44:09, John wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:647: void set_module(wasm::ModuleEnv *Module) { this->module_ = Module; } On 2016/03/28 18:44:10, John wrote: > setModule This name is set by V8, so I can't change it. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:649: void set_control_ptr(Node *Control) { this->control_ = Control; } On 2016/03/28 18:44:09, John wrote: > setControlPtr This name is set by V8, so I can't change it. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:651: void set_effect_ptr(Node *Effect) { this->effect_ = Effect; } On 2016/03/28 18:44:08, John wrote: > setEffectPtr This name is set by V8, so I can't change it. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:681: uint8_t Buffer[BUFFER_SIZE]; On 2016/03/28 18:17:13, JF wrote: > Can you allocate this instead? Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:691: auto BytesRead = InputStream->GetBytes(Buffer, sizeof(Buffer)); On 2016/03/28 18:44:08, John wrote: > integral type? don't use auto? Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:740: auto NewName = fnNameFromId(Id++); On 2016/03/28 18:44:07, John wrote: > std::string? Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.cpp#... src/WasmTranslator.cpp:751: auto Cfg = On 2016/03/28 18:44:07, John wrote: > auto * translateFunction returns a std::unique_ptr, so auto * doesn't work here. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h File src/WasmTranslator.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:1: #pragma once On 2016/03/28 18:17:14, JF wrote: > Copyright notice for the file. > > Also, pragma once? Include guards are standard. Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:8: namespace v8::internal { On 2016/03/28 18:44:10, John wrote: > we tend to do > > namespace v8 { > namespace internal { Done. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:8: namespace v8::internal { On 2016/03/28 18:44:10, John wrote: > I would just #include the files defining these... I decided to keep it as is, at least for now. The reason is that it allows us to build without having the V8 source code available. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:9: class Zone; On 2016/03/28 18:44:10, John wrote: > no spaces? make sure clang-format is happy here. This was what `make format` did. https://codereview.chromium.org/1837663002/diff/40017/src/WasmTranslator.h#ne... src/WasmTranslator.h:34: std::unique_ptr<llvm::DataStreamer> &&InputStream); On 2016/03/28 18:44:10, John wrote: > std::unique_ptr<> InputStream (i.e., no r-value reference) Done. https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.py File wasm-run-torture-tests.py (right): https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.... wasm-run-torture-tests.py:1: #!/usr/bin/python3 On 2016/03/28 18:17:14, JF wrote: > I think the following is more common: > #!/usr/bin/env python3 > > Also, copyright. Done.
The latest changes address Jim's initial suggestions. https://codereview.chromium.org/1837663002/diff/40017/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/40017/Makefile.standalone#new... Makefile.standalone:273: CXXFLAGS += $(V8_CXXFLAGS) -DSUBZERO_WASM On 2016/03/29 17:49:56, stichnot wrote: > Follow the pattern above where you also modify OBJDIR. > > Also, I suggest -DALLOW_WASM to follow the other ALLOW_* defines. Done, assuming I understood the comment correctly. https://codereview.chromium.org/1837663002/diff/40017/Makefile.standalone#new... Makefile.standalone:425: $(V8_LIBS) On 2016/03/29 17:49:56, stichnot wrote: > untabify this line (or just add it to the previous line) Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.cpp#newcode235 src/IceCfg.cpp:235: const auto InEdges = Node->getInEdges(); On 2016/03/29 17:49:57, stichnot wrote: > Should this be "const auto &" ? I don't know. getInEdges() returns a ref, and > I'm not sure whether you're accidentally making a copy here. Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.cpp#newcode237 src/IceCfg.cpp:237: auto Phi = llvm::cast<InstPhi>(&Inst); On 2016/03/29 17:49:57, stichnot wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfg.h#newcode268 src/IceCfg.h:268: /// The WASM translater cannot always determine the right incoming edge for a On 2016/03/29 17:49:57, stichnot wrote: > translator Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.cpp#newc... src/IceCfgNode.cpp:45: if (auto Dest = Instr->getDest()) { On 2016/03/29 17:49:57, stichnot wrote: > auto * > here and below Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.h File src/IceCfgNode.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCfgNode.h#newcod... src/IceCfgNode.h:113: void addOutEdge(CfgNode *out) { OutEdges.push_back(out); } On 2016/03/29 17:49:57, stichnot wrote: > Capitalize Out and In vars per LLVM convention. Done. https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceCompiler.cpp#new... src/IceCompiler.cpp:100: #ifdef SUBZERO_WASM On 2016/03/29 17:49:57, stichnot wrote: > We try to minimize the use of #ifdef code. Where possible, the code is > conditional on some Ice::BuildDefs constexpr function so that mistakes are > caught by the compiler in early development. > > Would that be possible here? My latest update should have basically no #ifdef code anymore. https://codereview.chromium.org/1837663002/diff/40017/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1837663002/diff/40017/src/IceOperand.h#newcod... src/IceOperand.h:785: Inst *DefiningInst = nullptr; On 2016/03/29 17:49:57, stichnot wrote: > I'm not super excited about increasing the size of > operands/variable/instructions for what is currently wasm-specific info. > > Also, I don't see that this info is being updated when: > 1) the defining instruction changes e.g. during lowering when the original > defining instruction is deleted and a new one is inserted > 2) the defining node changes due to node splitting > > I think a more appropriate approach would be to maintain various unordered_maps > during parsing and then discard them after all is parsed. I added a couple unordered_maps and removed the extra fields. They allocate from the CFG right now, which may not be ideal, but it at least keeps the memory usage the same on non-WASM paths. https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.py File wasm-run-torture-tests.py (right): https://codereview.chromium.org/1837663002/diff/40017/wasm-run-torture-tests.... wasm-run-torture-tests.py:15: cmd = "LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target ./pnacl-sz -filetype=asm -target=arm32 {} -threads=0 -O2 -verbose=wasm".format(test_file) On 2016/03/29 17:49:57, stichnot wrote: > 80-col Done. > > Yucky absolute paths. I changed them to relative paths, although this layout is still less than ideal. > > -target=arm32??? Since I've mostly been working on the ARM target, I find that assembly easier to read, which is why I picked it here. > > Move this file into pydir/ so that it can later benefit from other utilities > there. Done.
https://codereview.chromium.org/1837663002/diff/160001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/160001/Makefile.standalone#ne... Makefile.standalone:182: V8_DIR = ../../../../v8 Yikes. Can this be specified in terms of $(NACL_ROOT) ? https://codereview.chromium.org/1837663002/diff/160001/Makefile.standalone#ne... Makefile.standalone:273: CXXFLAGS := $(CXXFLAGS) $(V8_CXXFLAGS) -DALLOW_WASM=1 also, something like: OBJDIR := $(OBJDIR)+Wasm https://codereview.chromium.org/1837663002/diff/160001/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/IceCfg.cpp#newcode248 src/IceCfg.cpp:248: for (auto &Inst : Node->getPhis()) { I would use "auto *Instr" here. https://codereview.chromium.org/1837663002/diff/160001/src/IceCfg.cpp#newcode415 src/IceCfg.cpp:415: assert(Node->getOutEdges().size() == 1 || This assert shows up at least a couple times, and it's not obvious at first glance what the condition represents. How about a helper bool function with a self-documenting name? https://codereview.chromium.org/1837663002/diff/160001/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/IceCfgNode.cpp#new... src/IceCfgNode.cpp:48: if (auto N = Br->getTargetFalse()) { auto *N https://codereview.chromium.org/1837663002/diff/160001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/IceCompiler.cpp#ne... src/IceCompiler.cpp:28: remove blank line? https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:37: remove leading blank line https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:40: llvm_unreachable("unexpected enum value"); Favor llvm::report_fatal_error over llvm_unreachable unless there's a good reason. IIRC, llvm_unreachable is more like assert() in that it may be compiled out in a release build. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:64: /** Subzero block commenting style is usually "//" prefixed lines. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:68: flow, etc. In Subzero these concepts are all separate. This class let's V8's lets https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:76: uintptr_t Data; Maybe use "uintptr_t Data = UNDEF_PTR;"? Then you can just use "OperandNode(...) = default;" for two of the ctors. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:79: OperandNode(Operand *Operand) : Data((uintptr_t)Operand) {} mark these ctors explicit https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:79: OperandNode(Operand *Operand) : Data((uintptr_t)Operand) {} I think this sort of casting would be better off as a reinterpret_cast or whatever. (Not so much the constexpr above, though.) https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:108: bool isCfgNode() const { return Data & NODE_FLAG; } So UNDEF_PTR is considered to be a CfgNode and not an Operand? That's oddly asymmetric. Without having read further into this file, I wonder if it's more appropriate for both methods to return false on UNDEF_PTR. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:113: auto Oper = static_cast<Operand *>(Op); auto * https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:113: auto Oper = static_cast<Operand *>(Op); There are an awful lot of static_casts in this file. What do you think of having toOperand(), toNode(), etc. helpers to factor out all those static_casts? https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:116: auto Node = static_cast<CfgNode *>(Op); auto * https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:146: #define LOG(Expr) log([&](Ostream & out) { Expr; }) I would move macro definitions like this closer to the top, since it's used in pretty much the whole file. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:156: IceBuilder(class Cfg *Cfg) explicit https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:156: IceBuilder(class Cfg *Cfg) Don't use a type name for a variable name. We used to do that a bit, and would sometimes get into trouble when certain patterns of new code were added, or maybe g++ would be more picky, or ... In any case, Subzero usually does "Cfg *Func". https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:160: Node *Buffer(size_t Count) { All these method names should start with lowercase. Sorry. Or are you cloning a V8 or WebAsm API? https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:166: Node Start(unsigned params) { capitalize Params https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:178: NextArg++; I think it's best to just get used to pre-incrementing everywhere, unless you explicitly need post-increment. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:216: const auto InEdges = static_cast<CfgNode *>(Control)->getInEdges(); const auto & (I think) https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:227: // constant. Yeah -- here's where you eventually might just have to hold the phi data off to the side until the CFG stabilizes. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:279: auto *Dest = makeVariable(isComparison(Opcode) Just checking - does wasm allow variables and functions to have explicit names? I see a lot of makeVariable() calls but no setName(). https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:384: auto *Zero = makeVariable(IceType_f32); auto *_0 = Ctx->getConstantZero(IceType_f32); Control()->appendInst(InstArithmetic::create(Cfg, InstArithmetic::Fsub, Dest, _0, Input)); I.e., you don't need to create a special variable that has 0 assigned to it. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:414: LOG(out << "IsPhiWithMerge(" << Phi << ", " << Merge << ")" Did you run "make format"? I'm surprised this statement spans two lines. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:529: // assert(Module->IsValidFunction(index)); remove this? https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:549: auto *Call = InstCall::create(Cfg, NumArgs, Dest, TargetOperand, Subzero tends to document constant arguments like: constexpr bool NoTailCall = false; InstCall::create(..., NoTailCall); https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:579: // TODO(eholk): surely there is a better way to do this. Probably not - LLVM/PNaCl representation is pretty low-level here... https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:640: void set_module(wasm::ModuleEnv *Module) { this->Module = Module; } The LLVM/Subzero naming convention would be more like setModule(). https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:692: OstreamLocker L(Ctx); This and the LOG() macro are clever. The following is optional for this CL. For verbose output at this level, I think you really want to hold the Ostream lock for the entire duration of parsing one function (and of course only in a DUMP-enabled build). Otherwise, when we get to the point where we let the translation threads also parse in parallel, you end up with hopelessly interleaved parse log output. This approach is already done elsewhere, including (off the top of my head) register allocation and address mode inference. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:734: delete[] Buffer; Can Buffer be a unique_ptr? Then you don't need to bother deleting it here. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:812: LOG(out << "done." Do one of these instead: out << "done.\n" out << "done." "\n" (the latter being compiler string concatenation) https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h File src/WasmTranslator.h (right): https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:8: //===----------------------------------------------------------------------===// The license boilerplate usually also includes a blurb at the end about the high-level purpose of the file. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:34: OstreamLocker L(Ctx); The "new" Subzero style is to name this RAII variable "_" instead of "L", if practical. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:36: Ctx->getStrDump().flush(); Just curious if this flush() is necessary and if we should be doing that in strategic places elsewhere in the code base? I had been under the impression that "\n" would flush, but I could be wrong. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:47: std::unique_ptr<Cfg> translateFunction(v8::internal::Zone *zone, Capitalize variable names per LLVM convention. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:49: const uint8_t *base, Can you document these arguments, especially base/start/end?
https://codereview.chromium.org/1837663002/diff/160001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/160001/Makefile.standalone#ne... Makefile.standalone:182: V8_DIR = ../../../../v8 On 2016/04/01 01:46:43, stichnot wrote: > Yikes. > > Can this be specified in terms of $(NACL_ROOT) ? Done. https://codereview.chromium.org/1837663002/diff/160001/Makefile.standalone#ne... Makefile.standalone:273: CXXFLAGS := $(CXXFLAGS) $(V8_CXXFLAGS) -DALLOW_WASM=1 On 2016/04/01 01:46:43, stichnot wrote: > also, something like: > > OBJDIR := $(OBJDIR)+Wasm Done. https://codereview.chromium.org/1837663002/diff/160001/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/IceCfg.cpp#newcode248 src/IceCfg.cpp:248: for (auto &Inst : Node->getPhis()) { On 2016/04/01 01:46:43, stichnot wrote: > I would use "auto *Instr" here. I changed it to `auto &Instr`. It seems the instructions are stored by value in the list of phis, so I `auto *` doesn't work. https://codereview.chromium.org/1837663002/diff/160001/src/IceCfg.cpp#newcode415 src/IceCfg.cpp:415: assert(Node->getOutEdges().size() == 1 || On 2016/04/01 01:46:43, stichnot wrote: > This assert shows up at least a couple times, and it's not obvious at first > glance what the condition represents. How about a helper bool function with a > self-documenting name? Done. https://codereview.chromium.org/1837663002/diff/160001/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/IceCfgNode.cpp#new... src/IceCfgNode.cpp:48: if (auto N = Br->getTargetFalse()) { On 2016/04/01 01:46:43, stichnot wrote: > auto *N Done. https://codereview.chromium.org/1837663002/diff/160001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/IceCompiler.cpp#ne... src/IceCompiler.cpp:28: On 2016/04/01 01:46:44, stichnot wrote: > remove blank line? Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:37: On 2016/04/01 01:46:45, stichnot wrote: > remove leading blank line Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:40: llvm_unreachable("unexpected enum value"); On 2016/04/01 01:46:44, stichnot wrote: > Favor llvm::report_fatal_error over llvm_unreachable unless there's a good > reason. IIRC, llvm_unreachable is more like assert() in that it may be compiled > out in a release build. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:64: /** On 2016/04/01 01:46:44, stichnot wrote: > Subzero block commenting style is usually "//" prefixed lines. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:68: flow, etc. In Subzero these concepts are all separate. This class let's V8's On 2016/04/01 01:46:45, stichnot wrote: > lets Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:76: uintptr_t Data; On 2016/04/01 01:46:45, stichnot wrote: > Maybe use "uintptr_t Data = UNDEF_PTR;"? Then you can just use > "OperandNode(...) = default;" for two of the ctors. Done. The compiler still wouldn't accept default for the nullptr version though. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:79: OperandNode(Operand *Operand) : Data((uintptr_t)Operand) {} On 2016/04/01 01:46:44, stichnot wrote: > I think this sort of casting would be better off as a reinterpret_cast or > whatever. (Not so much the constexpr above, though.) Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:79: OperandNode(Operand *Operand) : Data((uintptr_t)Operand) {} On 2016/04/01 01:46:45, stichnot wrote: > mark these ctors explicit Done. This required a little bit more change on the V8 side than I really wanted, but oh well. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:108: bool isCfgNode() const { return Data & NODE_FLAG; } On 2016/04/01 01:46:45, stichnot wrote: > So UNDEF_PTR is considered to be a CfgNode and not an Operand? That's oddly > asymmetric. > > Without having read further into this file, I wonder if it's more appropriate > for both methods to return false on UNDEF_PTR. Good catch. Your suggestion was actually the intended behavior, and making that change didn't break anything. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:113: auto Oper = static_cast<Operand *>(Op); On 2016/04/01 01:46:44, stichnot wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:113: auto Oper = static_cast<Operand *>(Op); On 2016/04/01 01:46:45, stichnot wrote: > There are an awful lot of static_casts in this file. What do you think of > having toOperand(), toNode(), etc. helpers to factor out all those static_casts? I think John suggested this too, and I've now seen the wisdom of this approach. I'll make the change. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:116: auto Node = static_cast<CfgNode *>(Op); On 2016/04/01 01:46:44, stichnot wrote: > auto * Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:146: #define LOG(Expr) log([&](Ostream & out) { Expr; }) On 2016/04/01 01:46:44, stichnot wrote: > I would move macro definitions like this closer to the top, since it's used in > pretty much the whole file. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:156: IceBuilder(class Cfg *Cfg) On 2016/04/01 01:46:44, stichnot wrote: > explicit Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:156: IceBuilder(class Cfg *Cfg) On 2016/04/01 01:46:44, stichnot wrote: > Don't use a type name for a variable name. We used to do that a bit, and would > sometimes get into trouble when certain patterns of new code were added, or > maybe g++ would be more picky, or ... > > In any case, Subzero usually does "Cfg *Func". Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:160: Node *Buffer(size_t Count) { On 2016/04/01 01:46:44, stichnot wrote: > All these method names should start with lowercase. Sorry. > > Or are you cloning a V8 or WebAsm API? Right, these are called by V8, so we have to follow their naming conventions. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:166: Node Start(unsigned params) { On 2016/04/01 01:46:44, stichnot wrote: > capitalize Params Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:178: NextArg++; On 2016/04/01 01:46:44, stichnot wrote: > I think it's best to just get used to pre-incrementing everywhere, unless you > explicitly need post-increment. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:216: const auto InEdges = static_cast<CfgNode *>(Control)->getInEdges(); On 2016/04/01 01:46:44, stichnot wrote: > const auto & (I think) Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:227: // constant. On 2016/04/01 01:46:44, stichnot wrote: > Yeah -- here's where you eventually might just have to hold the phi data off to > the side until the CFG stabilizes. I was thinking about this some more, and it's possibly that multiplying by 2 might be enough to guarantee we always have enough space. The reason is that I think Wasm's AST structure seems to make it so no block ever has more than two incoming edges (at least I haven't seen any in my tests). At any rate, I'd like to hold off on fixing this for a later CL. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:279: auto *Dest = makeVariable(isComparison(Opcode) On 2016/04/01 01:46:44, stichnot wrote: > Just checking - does wasm allow variables and functions to have explicit names? > I see a lot of makeVariable() calls but no setName(). I think imports and exports have explicit names (and we use those), but just about everything else seems to be anonymous to me. I think there may be names early on but they have been removed by the time we get here. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:384: auto *Zero = makeVariable(IceType_f32); On 2016/04/01 01:46:44, stichnot wrote: > auto *_0 = Ctx->getConstantZero(IceType_f32); > Control()->appendInst(InstArithmetic::create(Cfg, InstArithmetic::Fsub, Dest, > _0, Input)); > > I.e., you don't need to create a special variable that has 0 assigned to it. Right, I don't know why I thought you had to do it this way. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:414: LOG(out << "IsPhiWithMerge(" << Phi << ", " << Merge << ")" On 2016/04/01 01:46:44, stichnot wrote: > Did you run "make format"? I'm surprised this statement spans two lines. I may have changed this since the last time I ran make format. I'll run again before I update. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:529: // assert(Module->IsValidFunction(index)); On 2016/04/01 01:46:44, stichnot wrote: > remove this? Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:549: auto *Call = InstCall::create(Cfg, NumArgs, Dest, TargetOperand, On 2016/04/01 01:46:45, stichnot wrote: > Subzero tends to document constant arguments like: > > constexpr bool NoTailCall = false; > InstCall::create(..., NoTailCall); Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:579: // TODO(eholk): surely there is a better way to do this. On 2016/04/01 01:46:44, stichnot wrote: > Probably not - LLVM/PNaCl representation is pretty low-level here... Okay, I'll remove the comment. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:640: void set_module(wasm::ModuleEnv *Module) { this->Module = Module; } On 2016/04/01 01:46:45, stichnot wrote: > The LLVM/Subzero naming convention would be more like setModule(). This is another function called by V8 so we have to use their name here. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:692: OstreamLocker L(Ctx); On 2016/04/01 01:46:45, stichnot wrote: > This and the LOG() macro are clever. Hopefully that's a good clever :) > > The following is optional for this CL. For verbose output at this level, I > think you really want to hold the Ostream lock for the entire duration of > parsing one function (and of course only in a DUMP-enabled build). Otherwise, > when we get to the point where we let the translation threads also parse in > parallel, you end up with hopelessly interleaved parse log output. > > This approach is already done elsewhere, including (off the top of my head) > register allocation and address mode inference. That seems like a good idea and a simple change, so I went ahead and did it. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:734: delete[] Buffer; On 2016/04/01 01:46:44, stichnot wrote: > Can Buffer be a unique_ptr? Then you don't need to bother deleting it here. I like that much better. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:812: LOG(out << "done." On 2016/04/01 01:46:45, stichnot wrote: > Do one of these instead: > > out << "done.\n" > out << "done." "\n" > > (the latter being compiler string concatenation) Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h File src/WasmTranslator.h (right): https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:8: //===----------------------------------------------------------------------===// On 2016/04/01 01:46:45, stichnot wrote: > The license boilerplate usually also includes a blurb at the end about the > high-level purpose of the file. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:34: OstreamLocker L(Ctx); On 2016/04/01 01:46:45, stichnot wrote: > The "new" Subzero style is to name this RAII variable "_" instead of "L", if > practical. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:36: Ctx->getStrDump().flush(); On 2016/04/01 01:46:45, stichnot wrote: > Just curious if this flush() is necessary and if we should be doing that in > strategic places elsewhere in the code base? I had been under the impression > that "\n" would flush, but I could be wrong. It's normally not necessary. Early on I had inserted flushes in a few key places (basically, where I thought the translator might crash before it got to a \n). When I factored all the logging code into a function/macro, I decided to conservatively flush everywhere. This code should only be present in dump-enabled builds, so I don't think it hurts to leave it in but I'm happy to remove it if you'd prefer. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:47: std::unique_ptr<Cfg> translateFunction(v8::internal::Zone *zone, On 2016/04/01 01:46:45, stichnot wrote: > Capitalize variable names per LLVM convention. Done. https://codereview.chromium.org/1837663002/diff/160001/src/WasmTranslator.h#n... src/WasmTranslator.h:49: const uint8_t *base, On 2016/04/01 01:46:45, stichnot wrote: > Can you document these arguments, especially base/start/end? Done. With the latest v8 changes (coming in my next CL), base, start and end have been combined into a single argument.
https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone#ne... Makefile.standalone:261: ifndef WASM Instead of ifndef / else / endif restructure as ifdef / else / endif https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone#ne... Makefile.standalone:741: FORMAT_BLACKLIST += ! -path "./wasm-install/*" I think the "*" at the end of the pattern is redundant, so remove? Then you could also remove the double-quotes. https://codereview.chromium.org/1837663002/diff/220001/README-wasm.md File README-wasm.md (right): https://codereview.chromium.org/1837663002/diff/220001/README-wasm.md#newcode6 README-wasm.md:6: LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target make -j48 -f Makefile.standalone WASM=1 && LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target ./pnacl-sz -O2 -filetype=asm -target=arm32 ./torture-s2wasm-sexpr-wasm/20000112-1.c.s.wast.wasm Do you really need to set LD_LIBRARY_PATH just to do the 'make'? Also, it would be super cool if you could figure out the magic to embed this path in the pnacl-sz binary. https://codereview.chromium.org/1837663002/diff/220001/README-wasm.md#newcode17 README-wasm.md:17: `wasm-run-torture-tests.py` can be used to run all the tests, or some reflow to 80-col? https://codereview.chromium.org/1837663002/diff/220001/pydir/wasm-run-torture... File pydir/wasm-run-torture-tests.py (right): https://codereview.chromium.org/1837663002/diff/220001/pydir/wasm-run-torture... pydir/wasm-run-torture-tests.py:1: #!/usr/bin/env python3 The rest of our python scripts specify python2. Can this one be changed to python2, or is there something python3-specific here? https://codereview.chromium.org/1837663002/diff/220001/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/1837663002/diff/220001/src/IceCfg.h#newcode261 src/IceCfg.h:261: /// The WASM translator cannot always determine the right incoming edge for a Might want to note that this is the case because the CFG is built incrementally. https://codereview.chromium.org/1837663002/diff/220001/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/220001/src/IceCfgNode.cpp#new... src/IceCfgNode.cpp:43: if (auto *Br = llvm::dyn_cast<InstBr>(Instr)) { You may also want to do the same for switch instructions. If it's a branch or switch (or any other terminator instruction), you could iterate over Instr->getTerminatorEdges() and add the out-edges. Also, I think it would be better if this block were guarded on wasm, e.g. BuildDefs::wasm(), or getFlags().wasm() or whatever it's called. https://codereview.chromium.org/1837663002/diff/220001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/220001/src/IceCompiler.cpp#ne... src/IceCompiler.cpp:83: const std::string &IRFilename = Flags.getIRFilename(); Safer long-term to use plain std::string instead of std::string& . https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:8: //===----------------------------------------------------------------------===// Add the blurb about what this file does. Probably you can move the blurb from the .h file to here, and replace the .h blurb with something like: Declares a driver for translating Wasm bitcode into PNaCl bitcode. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:16: Remove blank line? https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:19: #include "IceGlobalInits.h" alphabetize these https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:33: Ice::Type toIceType(v8::internal::MachineType) { Should these be inside an anonymous namespace? https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:77: explicit OperandNode() = default; You can remove "explicit" from a zero-argument method. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:84: operator Operand *() const { I'm not sure where operator() is actually used so it's hard to check more deeply on this, but I kind of expected this operator() to share its implementation with toOperand(). Same for the CfgNode version. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:111: Operand *toOperand() const { return static_cast<Operand *>(*this); } Hmm, I expected toOperand() and toNode() to make isOperand/isCfgNode checks first, and report_fatal_error if necessary. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:113: CfgNode *toNode() const { return static_cast<CfgNode *>(*this); } Name this toCfgNode() for consistency? https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:740: nullptr /* DecodeWasmModule ignores the Isolate Whatever *NoIsolate = nullptr; auto Result = DecodeWasmModule(NoIsolate, &Zone, Buffer.get()); and you'll save several more lines of hullabaloo. :) https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:743: * hullabaloo of making one. */, &Zone, Buffer.get(), 80-col
https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone#ne... Makefile.standalone:261: ifndef WASM On 2016/04/04 21:26:51, stichnot wrote: > Instead of > ifndef / else / endif > restructure as > ifdef / else / endif Done. https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone#ne... Makefile.standalone:741: FORMAT_BLACKLIST += ! -path "./wasm-install/*" On 2016/04/04 21:26:51, stichnot wrote: > I think the "*" at the end of the pattern is redundant, so remove? Then you > could also remove the double-quotes. I tried removing the * and it still grabbed all the files under that directory. I'll leave it in. https://codereview.chromium.org/1837663002/diff/220001/README-wasm.md File README-wasm.md (right): https://codereview.chromium.org/1837663002/diff/220001/README-wasm.md#newcode6 README-wasm.md:6: LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target make -j48 -f Makefile.standalone WASM=1 && LD_LIBRARY_PATH=~/nacl/v8/out/native/lib.target ./pnacl-sz -O2 -filetype=asm -target=arm32 ./torture-s2wasm-sexpr-wasm/20000112-1.c.s.wast.wasm On 2016/04/04 21:26:51, stichnot wrote: > Do you really need to set LD_LIBRARY_PATH just to do the 'make'? > > Also, it would be super cool if you could figure out the magic to embed this > path in the pnacl-sz binary. For some reason we do need the LD_LIBRARY_PATH even for make. I forget which step it is, but there is one that fails without this. I agree, it'd be better not to have to do this. Hopefully we can do this when we figure out the right way to pull V8 into the system. https://codereview.chromium.org/1837663002/diff/220001/README-wasm.md#newcode17 README-wasm.md:17: `wasm-run-torture-tests.py` can be used to run all the tests, or some On 2016/04/04 21:26:51, stichnot wrote: > reflow to 80-col? Done. https://codereview.chromium.org/1837663002/diff/220001/pydir/wasm-run-torture... File pydir/wasm-run-torture-tests.py (right): https://codereview.chromium.org/1837663002/diff/220001/pydir/wasm-run-torture... pydir/wasm-run-torture-tests.py:1: #!/usr/bin/env python3 On 2016/04/04 21:26:51, stichnot wrote: > The rest of our python scripts specify python2. Can this one be changed to > python2, or is there something python3-specific here? Done. https://codereview.chromium.org/1837663002/diff/220001/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/1837663002/diff/220001/src/IceCfg.h#newcode261 src/IceCfg.h:261: /// The WASM translator cannot always determine the right incoming edge for a On 2016/04/04 21:26:51, stichnot wrote: > Might want to note that this is the case because the CFG is built incrementally. Done. https://codereview.chromium.org/1837663002/diff/220001/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/220001/src/IceCfgNode.cpp#new... src/IceCfgNode.cpp:43: if (auto *Br = llvm::dyn_cast<InstBr>(Instr)) { On 2016/04/04 21:26:51, stichnot wrote: > You may also want to do the same for switch instructions. > > If it's a branch or switch (or any other terminator instruction), you could > iterate over Instr->getTerminatorEdges() and add the out-edges. > > Also, I think it would be better if this block were guarded on wasm, e.g. > BuildDefs::wasm(), or getFlags().wasm() or whatever it's called. Done. https://codereview.chromium.org/1837663002/diff/220001/src/IceCompiler.cpp File src/IceCompiler.cpp (right): https://codereview.chromium.org/1837663002/diff/220001/src/IceCompiler.cpp#ne... src/IceCompiler.cpp:83: const std::string &IRFilename = Flags.getIRFilename(); On 2016/04/04 21:26:51, stichnot wrote: > Safer long-term to use plain std::string instead of std::string& . Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:8: //===----------------------------------------------------------------------===// On 2016/04/04 21:26:51, stichnot wrote: > Add the blurb about what this file does. > > Probably you can move the blurb from the .h file to here, and replace the .h > blurb with something like: > > Declares a driver for translating Wasm bitcode into PNaCl bitcode. Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:16: On 2016/04/04 21:26:52, stichnot wrote: > Remove blank line? Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:19: #include "IceGlobalInits.h" On 2016/04/04 21:26:52, stichnot wrote: > alphabetize these Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:33: Ice::Type toIceType(v8::internal::MachineType) { On 2016/04/04 21:26:51, stichnot wrote: > Should these be inside an anonymous namespace? Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:77: explicit OperandNode() = default; On 2016/04/04 21:26:52, stichnot wrote: > You can remove "explicit" from a zero-argument method. Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:84: operator Operand *() const { On 2016/04/04 21:26:52, stichnot wrote: > I'm not sure where operator() is actually used so it's hard to check more deeply > on this, but I kind of expected this operator() to share its implementation with > toOperand(). Same for the CfgNode version. I think most every use of these operators has been replaced with a call to toOperand or toNode. The static_cast<...> in toOperand and toNode actually calls this operator, so we are sharing the implementation. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:111: Operand *toOperand() const { return static_cast<Operand *>(*this); } On 2016/04/04 21:26:52, stichnot wrote: > Hmm, I expected toOperand() and toNode() to make isOperand/isCfgNode checks > first, and report_fatal_error if necessary. I replaced the asserts in the cast operators to checks with llvm::report_fatal_error, so I'll consider this done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:113: CfgNode *toNode() const { return static_cast<CfgNode *>(*this); } On 2016/04/04 21:26:51, stichnot wrote: > Name this toCfgNode() for consistency? Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:740: nullptr /* DecodeWasmModule ignores the Isolate On 2016/04/04 21:26:51, stichnot wrote: > Whatever *NoIsolate = nullptr; > auto Result = DecodeWasmModule(NoIsolate, &Zone, Buffer.get()); > > and you'll save several more lines of hullabaloo. :) Done. https://codereview.chromium.org/1837663002/diff/220001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:743: * hullabaloo of making one. */, &Zone, Buffer.get(), On 2016/04/04 21:26:52, stichnot wrote: > 80-col Done.
otherwise lgtm https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/1837663002/diff/220001/Makefile.standalone#ne... Makefile.standalone:741: FORMAT_BLACKLIST += ! -path "./wasm-install/*" On 2016/04/04 22:23:22, Eric Holk wrote: > On 2016/04/04 21:26:51, stichnot wrote: > > I think the "*" at the end of the pattern is redundant, so remove? Then you > > could also remove the double-quotes. > > I tried removing the * and it still grabbed all the files under that directory. > I'll leave it in. Yeah, sorry, I was just plain wrong about that. https://codereview.chromium.org/1837663002/diff/240001/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/1837663002/diff/240001/src/IceCfg.h#newcode262 src/IceCfg.h:262: /// value due the CFG being build incrementally. The fixPhiNodes pass fills in built https://codereview.chromium.org/1837663002/diff/240001/src/IceCfg.h#newcode262 src/IceCfg.h:262: /// value due the CFG being build incrementally. The fixPhiNodes pass fills in due to the https://codereview.chromium.org/1837663002/diff/240001/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/240001/src/IceCfgNode.cpp#new... src/IceCfgNode.cpp:44: if (Instr->getKind() == Inst::Switch || Instr->getKind() == Inst::Br) { I think it would be more "standard" for the code base, and perhaps more future-proof, to use: if (llvm::isa<InstSwitch>(Instr) || llvm::isa<InstBr>(Instr)) { https://codereview.chromium.org/1837663002/diff/240001/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/240001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:74: } // End of anonymous namespace lowercase "end" https://codereview.chromium.org/1837663002/diff/240001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:754: v8::internal::Isolate *NoIsolate = nullptr; // DecodeWasmModule ignores this. sorry, I forgot to mention "constexpr".
Thanks. I'll merge with master again and then submit. https://codereview.chromium.org/1837663002/diff/240001/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/1837663002/diff/240001/src/IceCfg.h#newcode262 src/IceCfg.h:262: /// value due the CFG being build incrementally. The fixPhiNodes pass fills in On 2016/04/04 23:08:20, stichnot wrote: > built Done. https://codereview.chromium.org/1837663002/diff/240001/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1837663002/diff/240001/src/IceCfgNode.cpp#new... src/IceCfgNode.cpp:44: if (Instr->getKind() == Inst::Switch || Instr->getKind() == Inst::Br) { On 2016/04/04 23:08:20, stichnot wrote: > I think it would be more "standard" for the code base, and perhaps more > future-proof, to use: > > if (llvm::isa<InstSwitch>(Instr) || llvm::isa<InstBr>(Instr)) { Done. https://codereview.chromium.org/1837663002/diff/240001/src/WasmTranslator.cpp File src/WasmTranslator.cpp (right): https://codereview.chromium.org/1837663002/diff/240001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:74: } // End of anonymous namespace On 2016/04/04 23:08:20, stichnot wrote: > lowercase "end" Done. https://codereview.chromium.org/1837663002/diff/240001/src/WasmTranslator.cpp... src/WasmTranslator.cpp:754: v8::internal::Isolate *NoIsolate = nullptr; // DecodeWasmModule ignores this. On 2016/04/04 23:08:20, stichnot wrote: > sorry, I forgot to mention "constexpr". Done.
Description was changed from ========== Initial Subzero WASM prototype. BUG= ========== to ========== Initial Subzero WASM prototype. BUG= R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:260001) manually as 16f8061ca23653172ea4c1a43659bb0501b5b474 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
