Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
395193005:
Start processing function blocks in Subzero. (Closed)
|
Created:
6 years, 5 months ago by Karl Modified:
6 years, 3 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Visibility:
Public. |
DescriptionStart processing function blocks.
Handle binops and returns.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3894
R=jvoung@chromium.org, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=d6064a1
Patch Set 1 #Patch Set 2 : Not ready for review. #Patch Set 3 : Code ready for review. #
Total comments: 43
Patch Set 4 : Fix issues with patch set 3. #Patch Set 5 : Fix nits in patch set 4. #
Total comments: 49
Patch Set 6 : Fix issues in patch set 5. #Patch Set 7 : fix missed issues in patch set 5. #Patch Set 8 : Run format-diff #
Total comments: 11
Patch Set 9 : Fix issues in patch set 8. #Patch Set 10 : Fix formatting. #
Total comments: 19
Patch Set 11 : Fix issues in patch set 10. #
Total comments: 30
Patch Set 12 : Fix issues with patch set 11. #Patch Set 13 : Fix conditional break in switch statement. #
Total comments: 2
Patch Set 14 : Fix issues in patch set 13. #
Messages
Total messages: 23 (0 generated)
PTAL. Thanks.
Some initial comments. I still need to look through PNaClTranslator.cpp. https://codereview.chromium.org/395193005/diff/30001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTranslator.h#newc... src/IceTranslator.h:38: GlobalContext *getContext() { Can getContext be const? (Also, I'm surprised clang-format didn't put the whole definition if getContext() and getFlags() on a single line...) https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp File src/IceTypeConverter.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:1: //===- subzero/src/IceTypeTranslator.cpp - Convert Ice/LLVm Types ---------===// ICE/LLVM https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:10: // This file implements how to convert LLVM types to ICE types, and ICE types 2 spaces between implements and how :) https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:21: AddLlvmType(IceType_void, llvm::Type::getVoidTy(Context)); It would be great if this could be encoded in IceTypes.def, but sadly I can't think of a satisfying way to do it. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:58: return LlvmTypes.at(IceType_i1); Any reason not to use LlvmTypes[] instead of LlvmTypes.at(), like is done above in the TypeConverter ctor? Same comment for the two instances in the .h file. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h File src/IceTypeConverter.h (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:1: //===- subzero/src/IceTypeConverter.h - Convert Ice/LLVm Types --*- C++ -*-===// ICE/LLVM https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:15: #include "PNaClTranslator.h" Alphabetize this after Ice*.h if possible. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:127: report_fatal_error("More function blocks than defined function addresses"); 80-char
Some initial comments too https://codereview.chromium.org/395193005/diff/30001/src/IceConverter.cpp File src/IceConverter.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/IceConverter.cpp#new... src/IceConverter.cpp:58: LLVM2ICEConverter(Ice::GlobalContext *Ctx, LLVMContext& LlvmContext) Before we get too many instances of "Llvm", I'd like to bikeshed a bit... I personally prefer LLVM, since also matches the capitalization of things like LLVMContext, but I guess if you guys really prefer Llvm, then okay... https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp File src/IceTypeConverter.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:1: //===- subzero/src/IceTypeTranslator.cpp - Convert Ice/LLVm Types ---------===// TypeTranslator -> TypeConverter https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:10: // This file implements how to convert LLVM types to ICE types, and ICE types implements how -> implements how https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h File src/IceTypeConverter.h (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:1: //===- subzero/src/IceTypeConverter.h - Convert Ice/LLVm Types --*- C++ -*-===// LLVm -> LLVM https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:22: #include <map> Currently, src/IceDefs.h is the one place where <map> and <set> are included. Maybe stick with that and add a typedef there? Besides a map, also looks like there's a vector defined in this file. Jim? https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:59: llvm::Type *getLlvmVoidType() const { This doesn't appear to be used anywhere? https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:122: /// Returns the value id that should be associated with the the the the -> the https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:443: Ty = Context->convertToLlvmType(Ice::IceType_void); Is this mostly because it's cheaper to check the converter table than to use the LLVMContext? Or that it'll be easier to write this as a plain "Ice::IceType_void" later? Are all the uses of "Type::foo(...getLLVMContext())" meant to be changed? Otherwise, there are other cases where that's not rewritten. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:505: Ty = Type::getVoidTy(Context->getLLVMContext()); E.g., how come this isn't using the converter? https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:873: // Fix so that we can go on. Hmm, while this is still a bit early, what should the error model be? (a) abort immediately (b) perform these fixups and continue a bit more, then abort at some point How much less friendly is (a), if friendliness is the advantage of (b) over (a)? What are the other advantages of (b)? Batched testing of errors? What is the consequence of forgetting to do a fixup and then continuing? Using uninitialized memory? https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:905: bool checkIfIntArithmeticOp(Ice::InstArithmetic::OpKind Op, Type *OpTy) { Some of these functions take LLVM types. At some point these should work without the LLVM type right? https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1057: Op = Ice::InstArithmetic::Or; Xor https://codereview.chromium.org/395193005/diff/30001/tests_lit/llvm2ice_tests... File tests_lit/llvm2ice_tests/pnacl-reader.ll (right): https://codereview.chromium.org/395193005/diff/30001/tests_lit/llvm2ice_tests... tests_lit/llvm2ice_tests/pnacl-reader.ll:47: ; CHECK-NEXT: %__2 = or i32 %__1, %__0 This doesn't look like xor ?
https://codereview.chromium.org/395193005/diff/30001/src/IceConverter.cpp File src/IceConverter.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/IceConverter.cpp#new... src/IceConverter.cpp:58: LLVM2ICEConverter(Ice::GlobalContext *Ctx, LLVMContext& LlvmContext) On 2014/07/17 23:57:55, jvoung wrote: > Before we get too many instances of "Llvm", I'd like to bikeshed a bit... I > personally prefer LLVM, since also matches the capitalization of things like > LLVMContext, but I guess if you guys really prefer Llvm, then okay... Consensus is to use LLVM. Changing. https://codereview.chromium.org/395193005/diff/30001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTranslator.h#newc... src/IceTranslator.h:38: GlobalContext *getContext() { On 2014/07/17 23:00:33, stichnot wrote: > Can getContext be const? > > (Also, I'm surprised clang-format didn't put the whole definition if > getContext() and getFlags() on a single line...) Don't know why this wasn't converted. Ran clang-format on file and fixed space of this (and next) method. Also added const (for this). https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp File src/IceTypeConverter.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:1: //===- subzero/src/IceTypeTranslator.cpp - Convert Ice/LLVm Types ---------===// On 2014/07/17 23:00:33, stichnot wrote: > ICE/LLVM Done. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:1: //===- subzero/src/IceTypeTranslator.cpp - Convert Ice/LLVm Types ---------===// On 2014/07/17 23:57:55, jvoung wrote: > TypeTranslator -> TypeConverter Done. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:10: // This file implements how to convert LLVM types to ICE types, and ICE types On 2014/07/17 23:00:33, stichnot wrote: > 2 spaces between implements and how :) Done. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:10: // This file implements how to convert LLVM types to ICE types, and ICE types On 2014/07/17 23:57:55, jvoung wrote: > implements how -> > implements how Done. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:21: AddLlvmType(IceType_void, llvm::Type::getVoidTy(Context)); On 2014/07/17 23:00:33, stichnot wrote: > It would be great if this could be encoded in IceTypes.def, but sadly I can't > think of a satisfying way to do it. Neither can I. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.cpp... src/IceTypeConverter.cpp:58: return LlvmTypes.at(IceType_i1); On 2014/07/17 23:00:33, stichnot wrote: > Any reason not to use LlvmTypes[] instead of LlvmTypes.at(), like is done above > in the TypeConverter ctor? Same comment for the two instances in the .h file. Fixing to use "[]", since during the constructor we checked if all ICE types were added. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h File src/IceTypeConverter.h (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:1: //===- subzero/src/IceTypeConverter.h - Convert Ice/LLVm Types --*- C++ -*-===// On 2014/07/17 23:00:33, stichnot wrote: > ICE/LLVM Done. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:1: //===- subzero/src/IceTypeConverter.h - Convert Ice/LLVm Types --*- C++ -*-===// On 2014/07/17 23:57:55, jvoung wrote: > LLVm -> LLVM Done. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:22: #include <map> On 2014/07/17 23:57:55, jvoung wrote: > Currently, src/IceDefs.h is the one place where <map> and <set> are included. > Maybe stick with that and add a typedef there? > > Besides a map, also looks like there's a vector defined in this file. > > Jim? Talked with Jim. Agreed to let IceDefs.h do the inclusion. https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:59: llvm::Type *getLlvmVoidType() const { On 2014/07/17 23:57:55, jvoung wrote: > This doesn't appear to be used anywhere? Removed. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:15: #include "PNaClTranslator.h" On 2014/07/17 23:00:33, stichnot wrote: > Alphabetize this after Ice*.h if possible. I intentionally placed this header file first, following the style convention of always including the corresponding .h file first to verify it has all needed include files. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:122: /// Returns the value id that should be associated with the the On 2014/07/17 23:57:56, jvoung wrote: > the the -> the Done. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:127: report_fatal_error("More function blocks than defined function addresses"); On 2014/07/17 23:00:33, stichnot wrote: > 80-char Done. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:443: Ty = Context->convertToLlvmType(Ice::IceType_void); On 2014/07/17 23:57:56, jvoung wrote: > Is this mostly because it's cheaper to check the converter table than to use the > LLVMContext? > > Or that it'll be easier to write this as a plain "Ice::IceType_void" later? > > Are all the uses of "Type::foo(...getLLVMContext())" meant to be changed? > Otherwise, there are other cases where that's not rewritten. What I was trying to do is, whenever possible, always describe the types in terms of ICE types, for consistency. While the use of LLVM type creators may be relatively fast (via the LLVM context), there are no guarantees. On the other hand, when using the TypeConverter, we know it is nothing more than indexing a vector. I've gone ahead and fixed all other cases (that I could) find, where there is a corresponding Ice::Type to use. There are two cases where I can't do this: (1) Function signatures, since Ice::Type doesn't implement this. (2) Global variable initialization, which uses a byte array. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:505: Ty = Type::getVoidTy(Context->getLLVMContext()); On 2014/07/17 23:57:56, jvoung wrote: > E.g., how come this isn't using the converter? Fixed. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:873: // Fix so that we can go on. On 2014/07/17 23:57:56, jvoung wrote: > Hmm, while this is still a bit early, what should the error model be? > > (a) abort immediately > (b) perform these fixups and continue a bit more, then abort at some point > > How much less friendly is (a), if friendliness is the advantage of (b) over (a)? > What are the other advantages of (b)? Batched testing of errors? > > What is the consequence of forgetting to do a fixup and then continuing? Using > uninitialized memory? The goal of error recovery is to make it easier to incrementally grow. I don't need (necessarily) tweak examples down to the subset that is currently implemented if error recovery occurs. To deal with this, I'm adding a command line flag to control if error recovery is allowed. When false (the default), method Error will call report_fatal_error. When true, it will report the error and return. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:905: bool checkIfIntArithmeticOp(Ice::InstArithmetic::OpKind Op, Type *OpTy) { On 2014/07/17 23:57:55, jvoung wrote: > Some of these functions take LLVM types. At some point these should work without > the LLVM type right? That would be ideal. However, I'm not sure how we can do this since the code is also used by the PNaCl ABI verifier for LLVM code. I do agree that we should switch the top-level calls to use ICE types. So I don't forget, I'm doing that switch now. https://codereview.chromium.org/395193005/diff/30001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1057: Op = Ice::InstArithmetic::Or; On 2014/07/17 23:57:56, jvoung wrote: > Xor Done. https://codereview.chromium.org/395193005/diff/30001/tests_lit/llvm2ice_tests... File tests_lit/llvm2ice_tests/pnacl-reader.ll (right): https://codereview.chromium.org/395193005/diff/30001/tests_lit/llvm2ice_tests... tests_lit/llvm2ice_tests/pnacl-reader.ll:47: ; CHECK-NEXT: %__2 = or i32 %__1, %__0 On 2014/07/17 23:57:56, jvoung wrote: > This doesn't look like xor ? Done.
https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h File src/IceTypeConverter.h (right): https://codereview.chromium.org/395193005/diff/30001/src/IceTypeConverter.h#n... src/IceTypeConverter.h:22: #include <map> On 2014/07/17 23:57:55, jvoung wrote: > Currently, src/IceDefs.h is the one place where <map> and <set> are included. > Maybe stick with that and add a typedef there? > > Besides a map, also looks like there's a vector defined in this file. > > Jim? Karl and I discussed offline. I agree it's a good idea to isolate includes for things like containers in a single place, i.e. IceTypes.h. Along those lines, I also prefer that each Ice*.h file wouldn't include any other Ice*.h file except for IceTypes.h and IceDefs.h. There are some places where that's being violated because of functions defined in the .h file, so some cleanup will be needed for that. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:73: virtual bool Error(const std::string &Message) LLVM_OVERRIDE { Should be error() not Error() according to coding style. Also, can you document what the return value is supposed to mean? https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:77: if (!AllowErrorRecovery) report_fatal_error("Unable to continue"); I don't like "if (cond) stmt;" on a single line, and I suspect clang-format doesn't like it either. :) https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:798: FunctionParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser) Disable default copy ctor etc. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:831: // The corresponding llvm function. LLVM :) https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:834: std::vector<Ice::Operand *> LocalVars; This should probably be a vector of Variable*, not Operand*. Also, could you use Func->getVariables() instead? https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:836: std::vector<Ice::CfgNode*> Nodes; I don't think you need this, since it should be available as Func->getNodes(). https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:915: // Returns false if valid. Otherwise generates error message and I find the "return false if valid, true if invalid" semantics counterintuitive. It's understandable when a Unix-style return code is an int where 0 means success and nonzero means one of many possible failure codes. But for a bool return type, I think true should be success (unless the function name suggests otherwise, like checkIfNotIntArithmeticOp()). Also, consider naming these isValidFoo(), to align with the various PNaClABITypeChecker methods they are based on. This applies here and several other places. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:956: } else { http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:963: return false; This return statement is unreachable, right?
will read through more later today, a couple of things spotted below https://codereview.chromium.org/395193005/diff/70001/src/IceConverter.cpp File src/IceConverter.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/IceConverter.cpp#new... src/IceConverter.cpp:61: // model. leftover comment https://codereview.chromium.org/395193005/diff/70001/src/IceTargetLoweringX86... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:203: #define X(tag, size, align, elts, elty, str, flags) \ "\" for continuation can go near 80 col mark for consistency https://codereview.chromium.org/395193005/diff/70001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:216: #define X(tag, size, align, elts, elty, str, flags) \ same https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.def#newcode20 src/IceTypes.def:20: , flags */ \ maybe put the comma in the previous line? https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:983: if (Ice::isFloatingType(BaseTy)) { not isFloatingType ? https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:990: if (!PNaClABITypeChecker::isValidScalarType( When will this end up begin checked, if isFloatingType() already checks things?
https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.cpp#newcode90 src/IceTypes.cpp:90: static_cast<bool>(TypeAttributes[Index].TypeFlags & TypeFlagIsInteger); Does this need to be a bitset (w/ Flag & Bit), or could each of the variants just be mutually exclusive? At least the two cases right now are mutually exclusive. I guess if you wanted the vector type-ness to be a flag, then it could be here, but isVectorType() currently just checks the typeNumElements(). https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:132: if (DefiningFunctionsList.size() <= NumFunctionBlocks) nit: flipping the comparison and using >= seems to read better (the error message is "more function blocks than...") https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:824: Ice::Cfg *Func; Smart pointer for Func? What deletes Func? I guess if it's a smart pointer, then translateFcn() must be done before the Func/FunctionParser is destroyed. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1065: return checkIfIntArithmeticOp(Op, Ty); It should be possible to Shl on an integer vector as well? See tests_lit/llvm2ice_tests/vector-arith.ll https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1068: return checkIfIntArithmeticOp(Op, Ty); similar https://codereview.chromium.org/395193005/diff/70001/tests_lit/llvm2ice_tests... File tests_lit/llvm2ice_tests/pnacl-reader.ll (right): https://codereview.chromium.org/395193005/diff/70001/tests_lit/llvm2ice_tests... tests_lit/llvm2ice_tests/pnacl-reader.ll:1: ; Tests the current state of the translator using PNaCl bitcode. Should we start making new directories for lit tests? E.g., (1) a "Reader" directory (or Parser, or whatever you want to call it) (2) an "llvm2ice" directory for the original reader/converter. (3) a "CodeGen" directory Under CodeGen, we can have x86-32 or whatnot, or we can keep the same .ll files and add more RUN/CHECK lines as we add architectures. However, that's very different from (1) and (2).
https://codereview.chromium.org/395193005/diff/70001/src/IceConverter.cpp File src/IceConverter.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/IceConverter.cpp#new... src/IceConverter.cpp:61: // model. On 2014/07/22 18:40:18, jvoung wrote: > leftover comment Done. https://codereview.chromium.org/395193005/diff/70001/src/IceTargetLoweringX86... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:203: #define X(tag, size, align, elts, elty, str, flags) \ On 2014/07/22 18:40:18, jvoung wrote: > "\" for continuation can go near 80 col mark for consistency Done. https://codereview.chromium.org/395193005/diff/70001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:216: #define X(tag, size, align, elts, elty, str, flags) \ On 2014/07/22 18:40:18, jvoung wrote: > same Done. https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.cpp#newcode90 src/IceTypes.cpp:90: static_cast<bool>(TypeAttributes[Index].TypeFlags & TypeFlagIsInteger); On 2014/07/23 16:10:17, jvoung wrote: > Does this need to be a bitset (w/ Flag & Bit), or could each of the variants > just be mutually exclusive? At least the two cases right now are mutually > exclusive. I guess if you wanted the vector type-ness to be a flag, then it > could be here, but isVectorType() currently just checks the typeNumElements(). I would actually like to convert checks like "isValidIntOrIntVectorArithOp", "isValidIntOrIntVectorLogicalOp", and "isValidFloatOrFloatVectorArithOp" to be flag checks. That is why I defined them as bit sets rather than exclusive values. However, since that hasn't been done yet, I will wait to change it to a bitset when the change occurs. https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/395193005/diff/70001/src/IceTypes.def#newcode20 src/IceTypes.def:20: , flags */ \ On 2014/07/22 18:40:18, jvoung wrote: > maybe put the comma in the previous line? Done. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:73: virtual bool Error(const std::string &Message) LLVM_OVERRIDE { On 2014/07/21 22:25:22, stichnot wrote: > Should be error() not Error() according to coding style. > > Also, can you document what the return value is supposed to mean? This is an inherited method, and must follow the conventions it has. Did add comments. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:77: if (!AllowErrorRecovery) report_fatal_error("Unable to continue"); On 2014/07/21 22:25:21, stichnot wrote: > I don't like "if (cond) stmt;" on a single line, and I suspect clang-format > doesn't like it either. :) Fixed by running through clang-format. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:132: if (DefiningFunctionsList.size() <= NumFunctionBlocks) On 2014/07/23 16:10:17, jvoung wrote: > nit: flipping the comparison and using >= seems to read better (the error > message is "more function blocks than...") Done. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:798: FunctionParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser) On 2014/07/21 22:25:22, stichnot wrote: > Disable default copy ctor etc. Done. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:824: Ice::Cfg *Func; On 2014/07/23 16:10:17, jvoung wrote: > Smart pointer for Func? > What deletes Func? > > I guess if it's a smart pointer, then translateFcn() must be done before the > Func/FunctionParser is destroyed. Yes. We call translateFcn with it in the destructor, which takes over ownership. Updated comment in IceTranslator.h to clarify that ownership is being taken. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:831: // The corresponding llvm function. On 2014/07/21 22:25:22, stichnot wrote: > LLVM :) Done. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:834: std::vector<Ice::Operand *> LocalVars; On 2014/07/21 22:25:21, stichnot wrote: > This should probably be a vector of Variable*, not Operand*. > > Also, could you use Func->getVariables() instead? This vector is used to convert indices from the bitcode file to corresponding operands (including forward references which definitely would not be in Func->getVariables()). Fixing name to LocalOperands (to be consistent with vector contents) and adding comment to clarify. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:836: std::vector<Ice::CfgNode*> Nodes; On 2014/07/21 22:25:21, stichnot wrote: > I don't think you need this, since it should be available as Func->getNodes(). Done. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:915: // Returns false if valid. Otherwise generates error message and On 2014/07/21 22:25:22, stichnot wrote: > I find the "return false if valid, true if invalid" semantics counterintuitive. > It's understandable when a Unix-style return code is an int where 0 means > success and nonzero means one of many possible failure codes. But for a bool > return type, I think true should be success (unless the function name suggests > otherwise, like checkIfNotIntArithmeticOp()). > > Also, consider naming these isValidFoo(), to align with the various > PNaClABITypeChecker methods they are based on. > > This applies here and several other places. Changed the context of boolean functions (when possible) to return true if the condition is met, and false otherwise. The exceptions are for inherited functions which use the other convention (i.e. return true when error occurs). https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:956: } else { On 2014/07/21 22:25:21, stichnot wrote: > http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return Done. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:963: return false; On 2014/07/21 22:25:21, stichnot wrote: > This return statement is unreachable, right? Yes. But I've refactored the code to better match the "isValid" form of this. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:983: if (Ice::isFloatingType(BaseTy)) { On 2014/07/22 18:40:18, jvoung wrote: > not isFloatingType ? Yes. Fixed. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:990: if (!PNaClABITypeChecker::isValidScalarType( On 2014/07/22 18:40:18, jvoung wrote: > When will this end up begin checked, if isFloatingType() already checks things? At the moment yes. However, the question is whether we should call the PNaCl ABI checking fucntions to handle any changes we may add (such as a larger floating point type). However, I agree that this is unlikely. Removing. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1065: return checkIfIntArithmeticOp(Op, Ty); On 2014/07/23 16:10:17, jvoung wrote: > It should be possible to Shl on an integer vector as well? > > See tests_lit/llvm2ice_tests/vector-arith.ll I see. I'm slightly confused with the semantics. Renaming (and changing semantics) to "isValidIntOrIntVectorLogicalOp". The difference between Arithmetic and Logical is that i1 is also allowed. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1068: return checkIfIntArithmeticOp(Op, Ty); On 2014/07/23 16:10:17, jvoung wrote: > similar Done. https://codereview.chromium.org/395193005/diff/70001/tests_lit/llvm2ice_tests... File tests_lit/llvm2ice_tests/pnacl-reader.ll (right): https://codereview.chromium.org/395193005/diff/70001/tests_lit/llvm2ice_tests... tests_lit/llvm2ice_tests/pnacl-reader.ll:1: ; Tests the current state of the translator using PNaCl bitcode. On 2014/07/23 16:10:17, jvoung wrote: > Should we start making new directories for lit tests? > > E.g., > > (1) a "Reader" directory (or Parser, or whatever you want to call it) > (2) an "llvm2ice" directory for the original reader/converter. > (3) a "CodeGen" directory > > Under CodeGen, we can have x86-32 or whatnot, or we can keep the same .ll files > and add more RUN/CHECK lines as we add architectures. However, that's very > different from (1) and (2). > Creating Reader directory for this test. Also added more tests, covering all binary operators (excluding those that need bitcast operations, since they are not defined yet).
Otherwise looks okay https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:824: Ice::Cfg *Func; On 2014/07/23 20:22:21, Karl wrote: > On 2014/07/23 16:10:17, jvoung wrote: > > Smart pointer for Func? > > What deletes Func? > > > > I guess if it's a smart pointer, then translateFcn() must be done before the > > Func/FunctionParser is destroyed. > > Yes. We call translateFcn with it in the destructor, which takes over ownership. > Updated comment in IceTranslator.h to clarify that ownership is being taken. Okay. Something about running translateFcn() in the dtor, doesn't seem clean (a bit unexpected?). I guess we're not using c++ exceptions, so that eliminates the possibiilty of wanting to abort and clean up (w/ the dtor) w/out doing the translation. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:834: std::vector<Ice::Operand *> LocalVars; On 2014/07/23 20:22:21, Karl wrote: > On 2014/07/21 22:25:21, stichnot wrote: > > This should probably be a vector of Variable*, not Operand*. > > > > Also, could you use Func->getVariables() instead? > > This vector is used to convert indices from the bitcode file to corresponding > operands (including forward references which definitely would not be in > Func->getVariables()). Fixing name to LocalOperands (to be consistent with > vector contents) and adding comment to clarify. It can still be a std::vector<Ice::Variable *> (does the subtyping work out)? https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1065: return checkIfIntArithmeticOp(Op, Ty); On 2014/07/23 20:22:21, Karl wrote: > On 2014/07/23 16:10:17, jvoung wrote: > > It should be possible to Shl on an integer vector as well? > > > > See tests_lit/llvm2ice_tests/vector-arith.ll > > I see. I'm slightly confused with the semantics. Renaming (and changing > semantics) to "isValidIntOrIntVectorLogicalOp". The difference between > Arithmetic and Logical is that i1 is also allowed. I think you mean changing to isValidIntOrIntVectorArithOp (which is what you changed this shift op to check). This allows each vector element to be shifted by a different amount (specified by the second vector's elements). Good catch with the And/Or/Xor, changing that to check "logical" types. https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:132: if (NumFunctionBlocks > DefiningFunctionsList.size()) Should be >=, to preserve the original <= ? If NumFunctionBlocks == 1, DFL.size() == 1, will later index into DFL[1] ? https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:369: bool isValidRecordSizeNoMoreThan(unsigned UpperLimit, const char *RecordName) { 80 col? "AtMost" might be shorter https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:389: /// record that has incorrect size. ContextMessage (if not 0) nit: should we just use NULL in the new code, instead of 0? There's mix of it right now (e.g., 0 here, but NULL for NextInstVar()). https://codereview.chromium.org/395193005/diff/100016/tests_lit/reader_tests/... File tests_lit/reader_tests/binops.ll (right): https://codereview.chromium.org/395193005/diff/100016/tests_lit/reader_tests/... tests_lit/reader_tests/binops.ll:721: ; CHECK-NEXT: } Test shift ops too?
https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:834: std::vector<Ice::Operand *> LocalVars; On 2014/07/24 19:01:44, jvoung wrote: > On 2014/07/23 20:22:21, Karl wrote: > > On 2014/07/21 22:25:21, stichnot wrote: > > > This should probably be a vector of Variable*, not Operand*. > > > > > > Also, could you use Func->getVariables() instead? > > > > This vector is used to convert indices from the bitcode file to corresponding > > operands (including forward references which definitely would not be in > > Func->getVariables()). Fixing name to LocalOperands (to be consistent with > > vector contents) and adding comment to clarify. > > It can still be a std::vector<Ice::Variable *> (does the subtyping work out)? What I understood from Karl is that ultimately this will indeed contain Operands, i.e. Variables plus Constants, even though the current code only supplies Variables. Is that correct? https://codereview.chromium.org/395193005/diff/100016/src/IceTypes.h File src/IceTypes.h (right): https://codereview.chromium.org/395193005/diff/100016/src/IceTypes.h#newcode24 src/IceTypes.h:24: TypeFlagIsInteger = 0x1, Why these specific choices of enum value? If these are intended to be or-able bit flags, I suggest something like enum VerboseItem in IceDefs.h. If they are just supposed to be distinct nonzero values, add something like "TypeFlagInvalid = 0" and then don't assign values to the others, like in IceIntrinsics.h. Thinking ahead, we may also want to encode scalar versus vector into these flags.
https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:824: Ice::Cfg *Func; On 2014/07/24 19:01:44, jvoung wrote: > On 2014/07/23 20:22:21, Karl wrote: > > On 2014/07/23 16:10:17, jvoung wrote: > > > Smart pointer for Func? > > > What deletes Func? > > > > > > I guess if it's a smart pointer, then translateFcn() must be done before the > > > Func/FunctionParser is destroyed. > > > > Yes. We call translateFcn with it in the destructor, which takes over > ownership. > > Updated comment in IceTranslator.h to clarify that ownership is being taken. > > Okay. > > Something about running translateFcn() in the dtor, doesn't seem clean (a bit > unexpected?). I guess we're not using c++ exceptions, so that eliminates the > possibiilty of wanting to abort and clean up (w/ the dtor) w/out doing the > translation. Decided to move the cleanup/call to the ExitBlock method, so that we don't need to worry about c++ exceptions. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:834: std::vector<Ice::Operand *> LocalVars; On 2014/07/24 19:55:22, stichnot wrote: > On 2014/07/24 19:01:44, jvoung wrote: > > On 2014/07/23 20:22:21, Karl wrote: > > > On 2014/07/21 22:25:21, stichnot wrote: > > > > This should probably be a vector of Variable*, not Operand*. > > > > > > > > Also, could you use Func->getVariables() instead? > > > > > > This vector is used to convert indices from the bitcode file to > corresponding > > > operands (including forward references which definitely would not be in > > > Func->getVariables()). Fixing name to LocalOperands (to be consistent with > > > vector contents) and adding comment to clarify. > > > > It can still be a std::vector<Ice::Variable *> (does the subtyping work out)? > > What I understood from Karl is that ultimately this will indeed contain > Operands, i.e. Variables plus Constants, even though the current code only > supplies Variables. Is that correct? That is correct. https://codereview.chromium.org/395193005/diff/70001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:1065: return checkIfIntArithmeticOp(Op, Ty); On 2014/07/24 19:01:44, jvoung wrote: > On 2014/07/23 20:22:21, Karl wrote: > > On 2014/07/23 16:10:17, jvoung wrote: > > > It should be possible to Shl on an integer vector as well? > > > > > > See tests_lit/llvm2ice_tests/vector-arith.ll > > > > I see. I'm slightly confused with the semantics. Renaming (and changing > > semantics) to "isValidIntOrIntVectorLogicalOp". The difference between > > Arithmetic and Logical is that i1 is also allowed. > > > I think you mean changing to isValidIntOrIntVectorArithOp (which is what you > changed this shift op to check). This allows each vector element to be shifted > by a different amount (specified by the second vector's elements). > > Good catch with the And/Or/Xor, changing that to check "logical" types. Yes. My comment was incorrect. However the code looks right. https://codereview.chromium.org/395193005/diff/100016/src/IceTypes.h File src/IceTypes.h (right): https://codereview.chromium.org/395193005/diff/100016/src/IceTypes.h#newcode24 src/IceTypes.h:24: TypeFlagIsInteger = 0x1, On 2014/07/24 19:55:22, stichnot wrote: > Why these specific choices of enum value? > > If these are intended to be or-able bit flags, I suggest something like enum > VerboseItem in IceDefs.h. > > If they are just supposed to be distinct nonzero values, add something like > "TypeFlagInvalid = 0" and then don't assign values to the others, like in > IceIntrinsics.h. > > Thinking ahead, we may also want to encode scalar versus vector into these > flags. After talking to Jim, refactoring IceTypes.defs to be a sequence of bit values, and encode as appropriate when defining the check function. Hence, these enumerated values are not needed. Removing them. https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:132: if (NumFunctionBlocks > DefiningFunctionsList.size()) On 2014/07/24 19:01:44, jvoung wrote: > Should be >=, to preserve the original <= ? > > If NumFunctionBlocks == 1, DFL.size() == 1, will later index into DFL[1] ? Good catch. Thanks. https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:369: bool isValidRecordSizeNoMoreThan(unsigned UpperLimit, const char *RecordName) { On 2014/07/24 19:01:44, jvoung wrote: > 80 col? > > "AtMost" might be shorter Done. https://codereview.chromium.org/395193005/diff/100016/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:389: /// record that has incorrect size. ContextMessage (if not 0) On 2014/07/24 19:01:44, jvoung wrote: > nit: should we just use NULL in the new code, instead of 0? > > There's mix of it right now (e.g., 0 here, but NULL for NextInstVar()). Fixed.
https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcode70 src/IceTypes.cpp:70: // Verify Number vector elements consistent with IsVec. nit: "Verify Number vector" -> "Verify that the number of elements is consistent..." ? https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:165: } // end anonymous namespace I think it would be nicer to just have all the tables in a contiguous anonymous namespace. Or one table of structs, where you can initialize the fields of the structs based on the table elements (haven't visualized if that would be too confusing or not though)... and the getter would be like TypeProps[Index].IsScalarInteger, TypeProps[Index].IsVectorInteger, etc. ? https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:177: #define X(tag, IsVec, IsInt, IsFloat) IsInt &&IsVec, space between && and IsVec https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:211: #define X(tag, IsVec, IsInt, IsFloat) IsFloat &&IsVec, space between && and IsVec https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def#newcode38 src/IceTypes.def:38: //Dictionary: nit: space between // and Dictionary (at least some style guides require that) https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def#newcode41 src/IceTypes.def:41: // F - Is floating point value (scalar or vector). pretty https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def#newcode58 src/IceTypes.def:58: X(IceType_v4f32, 1, 1, 1) \ v4f32 is not an integer vector https://codereview.chromium.org/395193005/diff/160001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/160001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:930: if (Ice::isScalarIntegerType(BaseTy)) { Could get rid of BaseTy and just have OpTy then. That looks like what you've done with the float version. https://codereview.chromium.org/395193005/diff/160001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:943: if (PNaClABITypeChecker::isValidVectorType( This check seems to be the same as VectorLogicalOp... Does this allow vectors of i1 then? I don't think it's allowed right?
https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcode70 src/IceTypes.cpp:70: // Verify Number vector elements consistent with IsVec. On 2014/07/25 22:42:08, jvoung wrote: > nit: "Verify Number vector" -> "Verify that the number of elements is > consistent..." ? Done. https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcode91 src/IceTypes.cpp:91: const size_t TypeAttributesSize = Removed this and replaced with llvm::array_lengthof https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:110: if (Index < TypeAttributesSize) { Simplified this (and subsequent functions) to not use else clause. https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:165: } // end anonymous namespace On 2014/07/25 22:42:09, jvoung wrote: > I think it would be nicer to just have all the tables in a contiguous anonymous > namespace. > > Or one table of structs, where you can initialize the fields of the structs > based on the table elements (haven't visualized if that would be too confusing > or not though)... and the getter would be like TypeProps[Index].IsScalarInteger, > TypeProps[Index].IsVectorInteger, etc. ? Merged into a single table of structs. https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:177: #define X(tag, IsVec, IsInt, IsFloat) IsInt &&IsVec, On 2014/07/25 22:42:08, jvoung wrote: > space between && and IsVec Done. https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:211: #define X(tag, IsVec, IsInt, IsFloat) IsFloat &&IsVec, On 2014/07/25 22:42:09, jvoung wrote: > space between && and IsVec Done. https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def#newcode38 src/IceTypes.def:38: //Dictionary: On 2014/07/25 22:42:09, jvoung wrote: > nit: space between // and Dictionary (at least some style guides require that) Done. https://codereview.chromium.org/395193005/diff/160001/src/IceTypes.def#newcode58 src/IceTypes.def:58: X(IceType_v4f32, 1, 1, 1) \ On 2014/07/25 22:42:09, jvoung wrote: > v4f32 is not an integer vector Done. https://codereview.chromium.org/395193005/diff/160001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/160001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:930: if (Ice::isScalarIntegerType(BaseTy)) { On 2014/07/25 22:42:09, jvoung wrote: > Could get rid of BaseTy and just have OpTy then. > > That looks like what you've done with the float version. Done. https://codereview.chromium.org/395193005/diff/160001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:943: if (PNaClABITypeChecker::isValidVectorType( On 2014/07/25 22:42:09, jvoung wrote: > This check seems to be the same as VectorLogicalOp... Does this allow vectors of > i1 then? I don't think it's allowed right? Cleaned up these isValid... methods to be simplier by making more type checking functions in IceTypes.h
https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode25 src/IceTypes.cpp:25: void xIceTypeMacroIntegrityCheck() { you might have to put an "__attribute__((unused))" to make the clang compiler happy w/ unused static functions https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode38 src/IceTypes.cpp:38: #define X(tag, IsVec, IsInt, IsFloat, IsIntAritArith) _props_table_tag_##tag, IsIntAritArith -> IsIntArith https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode71 src/IceTypes.cpp:71: // Verify that the number of vector elements consistent with IsVec. number of vector elements *is* consistent with IsVec ? https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:122: llvm_unreachable("Invalid type for typeWidthInBytes()"); Could you leave the return after llvm_unreachable? At least, some of the other code like typeString() still make an attempt to return something after an llvm_unreachable(), in the case that we compile with -D NDEBUG. We might want to decide which llvm_unreachable() should actually be report_fatal_error().
https://codereview.chromium.org/395193005/diff/180001/src/IceInst.cpp File src/IceInst.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceInst.cpp#newcode233 src/IceInst.cpp:233: return OpIndex < InstArithmeticAttributesSize Instead of "OpIndex < InstArithmeticAttributesSize", can you use "Op < InstArithmetic::_num" ? I recently cleaned out those table size consts in IceInstX8632, and may want to do the same in IceInst. https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.cpp File src/IceTypeConverter.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.cp... src/IceTypeConverter.cpp:40: assert(Ty == LLVMTypes.size()); May need static_cast<size_t>(Ty) to avoid warnings/errors on some platforms, see other instances of "static_cast<size_t>" in the code base. https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.h File src/IceTypeConverter.h (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.h#... src/IceTypeConverter.h:38: // Note: We use "at" here to verify Ty is valid. How about: We use "at" here in case Ty wasn't registered. ? https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.h#... src/IceTypeConverter.h:57: /// Returns LLVM integer type with number of bits. Returns NULL with *specified* number https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode25 src/IceTypes.cpp:25: void xIceTypeMacroIntegrityCheck() { On 2014/07/31 16:18:29, jvoung (OoO 8-1 to 11) wrote: > you might have to put an "__attribute__((unused))" to make the clang compiler > happy w/ unused static functions Definitely, as this had to be added in IceTargetLoweringX8632.cpp. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode86 src/IceTypes.cpp:86: static const struct TypeAttributeFields TypeAttributes[] = { Don't need "static" inside an anonymous namespace. Here and below. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:120: if (Index < llvm::array_lengthof(TypeAttributes)) For all instances of llvm::array_lengthof(TypeAttributes) and llvm::array_lengthof(TypePropertiesTable), here and below, can they be simplified to IceType_NUM? (and as a side effect, probably Index can be replaced with Ty) And, if llvm::array_lengthof isn't used, then the tables can be defined in terms of unnamed structs. BTW, on further inspection, I see there is more widespread cleanup of this nature that I'd like to do, so feel free to leave it as is and then everything can be done at once. https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:198: if (IceTy == Ice::IceType_NUM) { Maybe use >= instead of == to be safe? https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:263: // Reports that there is no corresponding ICE type for LLVMTy. Reports ... LLVMTy, and returns Ice::IceType_void. https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:458: break; If this is code that was ported/translated from the PNaCl repo, then ignore this comment. Otherwise: A conditional break inside a switch statement seems fragile. I think it would be better to rewrite code (here and below) like this: if (isValidRecordSize(0, "Type void")) { Ty = Context->convertToLLVMType(Ice::IceType_void); } break; https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:913: Ice::Type OpTy); Weird indent
https://codereview.chromium.org/395193005/diff/180001/src/IceInst.cpp File src/IceInst.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceInst.cpp#newcode233 src/IceInst.cpp:233: return OpIndex < InstArithmeticAttributesSize On 2014/08/01 18:05:33, stichnot wrote: > Instead of "OpIndex < InstArithmeticAttributesSize", can you use "Op < > InstArithmetic::_num" ? I recently cleaned out those table size consts in > IceInstX8632, and may want to do the same in IceInst. Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.cpp File src/IceTypeConverter.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.cp... src/IceTypeConverter.cpp:40: assert(Ty == LLVMTypes.size()); On 2014/08/01 18:05:33, stichnot wrote: > May need static_cast<size_t>(Ty) to avoid warnings/errors on some platforms, see > other instances of "static_cast<size_t>" in the code base. Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.h File src/IceTypeConverter.h (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.h#... src/IceTypeConverter.h:38: // Note: We use "at" here to verify Ty is valid. On 2014/08/01 18:05:33, stichnot wrote: > How about: > We use "at" here in case Ty wasn't registered. > ? Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypeConverter.h#... src/IceTypeConverter.h:57: /// Returns LLVM integer type with number of bits. Returns NULL On 2014/08/01 18:05:33, stichnot wrote: > with *specified* number Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode25 src/IceTypes.cpp:25: void xIceTypeMacroIntegrityCheck() { On 2014/07/31 16:18:29, jvoung wrote: > you might have to put an "__attribute__((unused))" to make the clang compiler > happy w/ unused static functions Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode25 src/IceTypes.cpp:25: void xIceTypeMacroIntegrityCheck() { On 2014/08/01 18:05:33, stichnot wrote: > On 2014/07/31 16:18:29, jvoung (OoO 8-1 to 11) wrote: > > you might have to put an "__attribute__((unused))" to make the clang compiler > > happy w/ unused static functions > > Definitely, as this had to be added in IceTargetLoweringX8632.cpp. Acknowledged. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode38 src/IceTypes.cpp:38: #define X(tag, IsVec, IsInt, IsFloat, IsIntAritArith) _props_table_tag_##tag, On 2014/07/31 16:18:29, jvoung wrote: > IsIntAritArith -> IsIntArith Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode71 src/IceTypes.cpp:71: // Verify that the number of vector elements consistent with IsVec. On 2014/07/31 16:18:29, jvoung wrote: > number of vector elements *is* consistent with IsVec ? Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcode86 src/IceTypes.cpp:86: static const struct TypeAttributeFields TypeAttributes[] = { On 2014/08/01 18:05:34, stichnot wrote: > Don't need "static" inside an anonymous namespace. Here and below. Done. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:120: if (Index < llvm::array_lengthof(TypeAttributes)) On 2014/08/01 18:05:33, stichnot wrote: > For all instances of llvm::array_lengthof(TypeAttributes) and > llvm::array_lengthof(TypePropertiesTable), here and below, can they be > simplified to IceType_NUM? (and as a side effect, probably Index can be > replaced with Ty) > > And, if llvm::array_lengthof isn't used, then the tables can be defined in terms > of unnamed structs. > > BTW, on further inspection, I see there is more widespread cleanup of this > nature that I'd like to do, so feel free to leave it as is and then everything > can be done at once. Changed to use IceType_NUM. https://codereview.chromium.org/395193005/diff/180001/src/IceTypes.cpp#newcod... src/IceTypes.cpp:122: llvm_unreachable("Invalid type for typeWidthInBytes()"); On 2014/07/31 16:18:29, jvoung wrote: > Could you leave the return after llvm_unreachable? > > At least, some of the other code like typeString() still make an attempt to > return something after an llvm_unreachable(), in the case that we compile with > -D NDEBUG. We might want to decide which llvm_unreachable() should actually be > report_fatal_error(). Done. https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:198: if (IceTy == Ice::IceType_NUM) { On 2014/08/01 18:05:34, stichnot wrote: > Maybe use >= instead of == to be safe? Done. https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:263: // Reports that there is no corresponding ICE type for LLVMTy. On 2014/08/01 18:05:34, stichnot wrote: > Reports ... LLVMTy, and returns Ice::IceType_void. Done. https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:458: break; On 2014/08/01 18:05:34, stichnot wrote: > If this is code that was ported/translated from the PNaCl repo, then ignore this > comment. Otherwise: > > A conditional break inside a switch statement seems fragile. I think it would > be better to rewrite code (here and below) like this: > > if (isValidRecordSize(0, "Type void")) { > Ty = Context->convertToLLVMType(Ice::IceType_void); > } > break; In much of the code in the PNaCl repo, the cases are a series of error checks, followed by code to do if none of the error checks apply. I would like to keep this consistent, and not changing. However, after looking at this code again. We probably should give up processing the record if an error has already been handled. Replacing the early breaks with returns. https://codereview.chromium.org/395193005/diff/180001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:913: Ice::Type OpTy); On 2014/08/01 18:05:34, stichnot wrote: > Weird indent Done.
Otherwise LGTM. https://codereview.chromium.org/395193005/diff/220001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/220001/src/IceTypes.cpp#newcode17 src/IceTypes.cpp:17: // #include "llvm/ADT/STLExtras.h" Just remove if it's unneeded?
LGTM https://codereview.chromium.org/395193005/diff/100016/tests_lit/reader_tests/... File tests_lit/reader_tests/binops.ll (right): https://codereview.chromium.org/395193005/diff/100016/tests_lit/reader_tests/... tests_lit/reader_tests/binops.ll:721: ; CHECK-NEXT: } On 2014/07/24 19:01:44, jvoung wrote: > Test shift ops too? Remember to test shift ops in the next CL, then?
Jan, do you have any additional feedback on this CL? Or, should I assume Jim's LGTM (after removing commmented out line) is good enough?
https://codereview.chromium.org/395193005/diff/100016/tests_lit/reader_tests/... File tests_lit/reader_tests/binops.ll (right): https://codereview.chromium.org/395193005/diff/100016/tests_lit/reader_tests/... tests_lit/reader_tests/binops.ll:721: ; CHECK-NEXT: } On 2014/07/24 19:01:44, jvoung wrote: > Test shift ops too? Added tests.
https://codereview.chromium.org/395193005/diff/220001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/395193005/diff/220001/src/IceTypes.cpp#newcode17 src/IceTypes.cpp:17: // #include "llvm/ADT/STLExtras.h" On 2014/08/27 20:47:26, stichnot wrote: > Just remove if it's unneeded? Done.
Message was sent while issue was closed.
Committed patchset #14 (id:240001) manually as d6064a1 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
