Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
353553004:
Add support for vector types and vector constants. (Closed)
|
Created:
6 years, 6 months ago by wala Modified:
6 years, 5 months ago CC:
native-client-reviews_googlegroups.com, JF Base URL:
https://gerrit.chromium.org/gerrit/p/native_client/pnacl-subzero.git@master Visibility:
Public. |
DescriptionAdd support for vector types.
- Add vector types to the type table.
- Add support for parsing vector types in llvm2ice.
- Legalize undef vector values to zero. Test that undef vector values are lowered correctly.
BUG=none
R=jvoung@chromium.org, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=928f129
Patch Set 1 #
Total comments: 12
Patch Set 2 : 1) Fix alignment in type table. 2) Add VECT128_BYTES constant. 3) add _movp() function. #
Total comments: 30
Patch Set 3 : Implement changes mentioned in comments. #
Total comments: 2
Patch Set 4 : 1) Remove StringList. 2) Assign redundant assign TODO to stichnot. 3) Fix RUN line. #
Total comments: 14
Patch Set 5 : Remove support for vector constants. #
Total comments: 2
Patch Set 6 : Add a TODO about return values of vector type. #
Total comments: 6
Patch Set 7 : Print out types in error messages. #
Total comments: 2
Messages
Total messages: 20 (0 generated)
+cc JF if you want to offer some thoughts too Otherwise, here's my initial pass. https://codereview.chromium.org/353553004/diff/1/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceDefs.h#newcode71 src/IceDefs.h:71: typedef llvm::SmallVector<bool, 16> BitVect; LLVM actually has a separate SmallBitVector<>: http://llvm.org/docs/ProgrammersManual.html#smallbitvector which is already being used in a few places in subzero. Or was there a reason to stick with smallvector instead? https://codereview.chromium.org/353553004/diff/1/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceGlobalContext.h#newco... src/IceGlobalContext.h:78: Constant *getConstantVector(Type Ty, Vect128 &Vector); const Vect128 & ? https://codereview.chromium.org/353553004/diff/1/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/353553004/diff/1/src/IceOperand.cpp#newcode259 src/IceOperand.cpp:259: assert(Vector.size() == 16); I think there are places in other files where you assert that the supported vector size is 16-bytes. Might be better to make a named constant variable for the 16, and use that here and in the below assert. Then it's more grep'able where we assume the size is 16... for some future time when we support bigger vectors. https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.cpp File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.c... src/IceTargetLoweringX8632.cpp:2362: _mov(Reg, Src0, Reg_xmm0); See question about reusing _mov or having a separate _movp https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.h File src/IceTargetLoweringX8632.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.h... src/IceTargetLoweringX8632.h:199: Context.insert(InstX8632Movp::create(Func, Dest, Src0)); I wonder if this should just be a separate _movp() function. Basically, are there cases where you *already* know that typeNumElements() > 1, in which case you can just call _movp() and avoid having _mov() do a check. https://codereview.chromium.org/353553004/diff/1/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/353553004/diff/1/src/IceTypes.def#newcode29 src/IceTypes.def:29: X(IceType_v4i1, 16, 16, 4, IceType_i1, "<4 x i1>") \ I think technically, PNaCl only requires that vectors be aligned up to the element size for the sake of loads and stores. Even if the LLVM IR originally says that the load/store for a pointer aligned to 16, our stabilization passes downgrade it to 4. That is to avoid some undefined behavior if the programmer made a mistake and didn't fully align the vector. That would mean that you'd have to use the movdqu or movups instead of the aligned variants to load/store them. On the other hand, when you generate the constant pools it's fine to align those 16 bytes. For i1, the alignment is ???(1?), but you can't load/store them for PNaCl anyway.
https://codereview.chromium.org/353553004/diff/1/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceDefs.h#newcode71 src/IceDefs.h:71: typedef llvm::SmallVector<bool, 16> BitVect; On 2014/06/26 00:45:40, jvoung wrote: > LLVM actually has a separate SmallBitVector<>: > http://llvm.org/docs/ProgrammersManual.html#smallbitvector > which is already being used in a few places in subzero. > > Or was there a reason to stick with smallvector instead? SmallVector has operator<. SmallBitVector does not. Constants are pooled using the TypePool<> class which stores the constant's value as part of a key in an internal map. Map keys need to ordered. https://codereview.chromium.org/353553004/diff/1/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceGlobalContext.h#newco... src/IceGlobalContext.h:78: Constant *getConstantVector(Type Ty, Vect128 &Vector); On 2014/06/26 00:45:40, jvoung wrote: > const Vect128 & > ? Nice catch, thanks. https://codereview.chromium.org/353553004/diff/1/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/353553004/diff/1/src/IceOperand.cpp#newcode259 src/IceOperand.cpp:259: assert(Vector.size() == 16); On 2014/06/26 00:45:41, jvoung wrote: > I think there are places in other files where you assert that the supported > vector size is 16-bytes. > > Might be better to make a named constant variable for the 16, and use that here > and in the below assert. Then it's more grep'able where we assume the size is > 16... for some future time when we support bigger vectors. That's a good idea. I will add it to IceDefs.h. The assert is mostly an artifact of the fact that it's using SmallVector. I was planning on using a fixed size array std::array<char, 16> when Subzero supports C++11. https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.h File src/IceTargetLoweringX8632.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.h... src/IceTargetLoweringX8632.h:199: Context.insert(InstX8632Movp::create(Func, Dest, Src0)); On 2014/06/26 00:45:41, jvoung wrote: > I wonder if this should just be a separate _movp() function. > > Basically, are there cases where you *already* know that typeNumElements() > 1, > in which case you can just call _movp() and avoid having _mov() do a check. I'll add a _movp function. However, there are a few cases where a type-agnostic _mov (which would emit the correct code to copy values between two registers or between a register and memory, regardless of the type of the operands) would be nice to have to keep the lowering code simple (such in in legalize or setArgOffsetAndCopy). https://codereview.chromium.org/353553004/diff/1/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/353553004/diff/1/src/IceTypes.def#newcode29 src/IceTypes.def:29: X(IceType_v4i1, 16, 16, 4, IceType_i1, "<4 x i1>") \ On 2014/06/26 00:45:41, jvoung wrote: > I think technically, PNaCl only requires that vectors be aligned up to the > element size for the sake of loads and stores. Even if the LLVM IR originally > says that the load/store for a pointer aligned to 16, our stabilization passes > downgrade it to 4. That is to avoid some undefined behavior if the programmer > made a mistake and didn't fully align the vector. > > That would mean that you'd have to use the movdqu or movups instead of the > aligned variants to load/store them. On the other hand, when you generate the > constant pools it's fine to align those 16 bytes. > > For i1, the alignment is ???(1?), but you can't load/store them for PNaCl > anyway. Okay, I'll change the entries.
https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.h File src/IceTargetLoweringX8632.h (right): https://codereview.chromium.org/353553004/diff/1/src/IceTargetLoweringX8632.h... src/IceTargetLoweringX8632.h:199: Context.insert(InstX8632Movp::create(Func, Dest, Src0)); On 2014/06/26 17:32:42, wala wrote: > On 2014/06/26 00:45:41, jvoung wrote: > > I wonder if this should just be a separate _movp() function. > > > > Basically, are there cases where you *already* know that typeNumElements() > > 1, > > in which case you can just call _movp() and avoid having _mov() do a check. > > I'll add a _movp function. However, there are a few cases where a type-agnostic > _mov (which would emit the correct code to copy values between two registers or > between a register and memory, regardless of the type of the operands) would be > nice to have to keep the lowering code simple (such in in legalize or > setArgOffsetAndCopy). Okay, yeah I was worried there might be some recursion w/ _mov and it's use to legalizeToVar() when Dest is NULL, and which might lead back to _mov, and it would all need to handle the types correctly. https://codereview.chromium.org/353553004/diff/20001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceInstX8632.cpp#new... src/IceInstX8632.cpp:245: bool InstX8632Movp::isRedundantAssign() const { At some point, we should just have a base implementation of isRedundantAssign, since these 3 definitions are the same, modulo the TODO comment. I'm guilty of adding the first of the duplicates, but yeah... https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp#newco... src/IceOperand.cpp:285: assert(Window == Vector.data() + 16); Could use VECT128_BYTES here too. https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h#newcode114 src/IceOperand.h:114: // ConstantValue<> wraps a concrete constant value nit: keep the period at the end of the sentence. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:729: Ostream &Str = Ctx->getStrEmit(); You could just make a local variable "Align = 16" and use that to denote the meanings. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:734: ConstantList Vectors = Ctx->getConstantPool(IceType_v8i16); Could add a comment that all non-i1 vectors are in the same constant pool (so the choice of IceType_v8i16 is semi-arbitrary). Similar for the IceType_v4i1 below. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:2489: if (typeNumElements(Ty) > 1) { Maybe there should be some inlineable "isVectorType(Ty)" function that conveys the purpose of the check more directly, rather than checking that the number of elements > 1. https://codereview.chromium.org/353553004/diff/20001/src/llvm2ice.cpp File src/llvm2ice.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/llvm2ice.cpp#newcode122 src/llvm2ice.cpp:122: } else if (const ConstantDataVector *CDV = Perhaps we should leave a comment that this block of code is not for string PNaCl bitcode -- or have a flag to utilize it in tests, so that it's more clear what code is ultimately needed by the backend. In the end, I guess Karl will change llvm2ice to read bitcode and generate Ice directly, skipping LLVM types. https://codereview.chromium.org/353553004/diff/20001/tests_lit/llvm2ice_tests... File tests_lit/llvm2ice_tests/vector-pool.ll (right): https://codereview.chromium.org/353553004/diff/20001/tests_lit/llvm2ice_tests... tests_lit/llvm2ice_tests/vector-pool.ll:8: define <4 x float> @test_v4f32(i32 %arg) { Might as well check an integer typed const pool value as well?
https://codereview.chromium.org/353553004/diff/20001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/20001/src/IceDefs.h#newcode69 src/IceDefs.h:69: // TODO(wala): Switch to std::array after C++11 Assign this TODO to me instead, not sure you'll get to it. :) https://codereview.chromium.org/353553004/diff/20001/src/IceDefs.h#newcode70 src/IceDefs.h:70: typedef llvm::SmallVector<char, 16> Vect128; Can/should you use VECT128_BYTES instead of 16 on these two lines? https://codereview.chromium.org/353553004/diff/20001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceGlobalContext.cpp... src/IceGlobalContext.cpp:255: Vect128 Zeros(16); I don't understand why Zeros is defined with 16 elements but typeNumElements() above. Also, can we use VECT128_BYTES instead of 16? https://codereview.chromium.org/353553004/diff/20001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceInstX8632.cpp#new... src/IceInstX8632.cpp:702: // TODO(wala): movups works with all vector operands, but there exist maybe TODO(wala,stichnot) just in case https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp#newco... src/IceOperand.cpp:251: // Convert a vector to a list of strings. Seems overly complicated to build up a StringList and then just iterate through it and dump the elements. Can you simplify by just passing an OStream& argument? (A slight function renaming would be in order too.) https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp#newco... src/IceOperand.cpp:288: } "end of anonymous namespace" comment https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp#newco... src/IceOperand.cpp:308: for (unsigned I = 0; I != NumElements;) { This loop would probably be more "natural" written like this: for (unsigned I = 0; I < NumElements; ++I) { if (I > 0) Str << ", "; Str << "i1 " << (Value[I] ? "1" : "0"); } Same for the ConstantVector::dump() loop above. https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h#newcode126 src/IceOperand.h:126: virtual void emit(GlobalContext *Ctx) const { (void)Ctx; } Assuming this is just a stub that the target lowering is expected to reimplement, can we just declare and not define the method, so that a missing target definition causes a link error? Or at least, make the implementation a call to llvm_unreachable(). https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:91: llvm::raw_string_ostream BaseOS(Result); I'd like to avoid unnecessarily building LLVM stream stuff into the Subzero core. I realize the ickiness of using snprintf to render strings from integers, as is done elsewhere in Subzero. In this case, you might be able to do something with xargs, using #elts to stringize the integer. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:729: Ostream &Str = Ctx->getStrEmit(); On 2014/06/26 23:33:46, jvoung wrote: > You could just make a local variable "Align = 16" and use that to denote the > meanings. Is it appropriate to reuse VECT128_BYTES for this purpose?
https://codereview.chromium.org/353553004/diff/20001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/20001/src/IceDefs.h#newcode70 src/IceDefs.h:70: typedef llvm::SmallVector<char, 16> Vect128; On 2014/06/27 18:30:16, stichnot wrote: > Can/should you use VECT128_BYTES instead of 16 on these two lines? It makes sense for Vect128, which should have 16 bytes and always be fixed width. BitVect is not fixed width. 16 is the largest possible value for the size of BitVect but I'm unsure if it's the right amount to preallocate. https://codereview.chromium.org/353553004/diff/20001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceGlobalContext.cpp... src/IceGlobalContext.cpp:255: Vect128 Zeros(16); On 2014/06/27 18:30:16, stichnot wrote: > I don't understand why Zeros is defined with 16 elements but typeNumElements() > above. > > Also, can we use VECT128_BYTES instead of 16? A BitVect, unlike a Vect128, can have a differing size depending on its type (4 x i1, 8 x i1, 16 x i1). A Vect128, as the name implies, is fixed width (although implemented currently with SmallVector<>, at least until a better implementation is found). VECT128_BYTES can be used here. https://codereview.chromium.org/353553004/diff/20001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceInstX8632.cpp#new... src/IceInstX8632.cpp:245: bool InstX8632Movp::isRedundantAssign() const { On 2014/06/26 23:33:46, jvoung wrote: > At some point, we should just have a base implementation of isRedundantAssign, > since these 3 definitions are the same, modulo the TODO comment. > > I'm guilty of adding the first of the duplicates, but yeah... I'll add a TODO... https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.cpp#newco... src/IceOperand.cpp:251: // Convert a vector to a list of strings. On 2014/06/27 18:30:16, stichnot wrote: > Seems overly complicated to build up a StringList and then just iterate through > it and dump the elements. Can you simplify by just passing an OStream& > argument? (A slight function renaming would be in order too.) If it's being emitted directly to an Ostream, I could just inline all of this into ConstantVector::dump. https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h#newcode126 src/IceOperand.h:126: virtual void emit(GlobalContext *Ctx) const { (void)Ctx; } On 2014/06/27 18:30:16, stichnot wrote: > Assuming this is just a stub that the target lowering is expected to > reimplement, can we just declare and not define the method, so that a missing > target definition causes a link error? Or at least, make the implementation a > call to llvm_unreachable(). Just declaring it works. There is enough variation in what the emit method needs to do that there isn't much sense in a default implementation of emit(). I'll also leave a comment that the target needs to specialize the emit() method for each particular constant value, as I was unsure at first about why there was a specialization implemented in TargetLoweringX8632.cpp. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:91: llvm::raw_string_ostream BaseOS(Result); On 2014/06/27 18:30:16, stichnot wrote: > I'd like to avoid unnecessarily building LLVM stream stuff into the Subzero > core. I realize the ickiness of using snprintf to render strings from integers, > as is done elsewhere in Subzero. In this case, you might be able to do > something with xargs, using #elts to stringize the integer. To avoid snprintf, this function could emit the value directly into an Ostream that it has been passed. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:729: Ostream &Str = Ctx->getStrEmit(); On 2014/06/27 18:30:16, stichnot wrote: > On 2014/06/26 23:33:46, jvoung wrote: > > You could just make a local variable "Align = 16" and use that to denote the > > meanings. > > Is it appropriate to reuse VECT128_BYTES for this purpose? I'll do this, and then explain that we naturally align vector constants although the PNaCl ABI doesn't require it. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:734: ConstantList Vectors = Ctx->getConstantPool(IceType_v8i16); On 2014/06/26 23:33:46, jvoung wrote: > Could add a comment that all non-i1 vectors are in the same constant pool (so > the choice of IceType_v8i16 is semi-arbitrary). Similar for the IceType_v4i1 > below. I will add the comment. I'm unsure how to avoid these arbitrary values given the IceGlobalContext interface. https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:2489: if (typeNumElements(Ty) > 1) { On 2014/06/26 23:33:46, jvoung wrote: > Maybe there should be some inlineable "isVectorType(Ty)" function that conveys > the purpose of the check more directly, rather than checking that the number of > elements > 1. I agree. https://codereview.chromium.org/353553004/diff/20001/src/llvm2ice.cpp File src/llvm2ice.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/llvm2ice.cpp#newcode122 src/llvm2ice.cpp:122: } else if (const ConstantDataVector *CDV = On 2014/06/26 23:33:46, jvoung wrote: > Perhaps we should leave a comment that this block of code is not for string > PNaCl bitcode -- or have a flag to utilize it in tests, so that it's more clear > what code is ultimately needed by the backend. > > In the end, I guess Karl will change llvm2ice to read bitcode and generate Ice > directly, skipping LLVM types. Also the next two blocks.
LGTM wrt my comments https://codereview.chromium.org/353553004/diff/40001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/40001/src/IceDefs.h#newcode68 src/IceDefs.h:68: typedef std::vector<IceString> StringList; I don't think you need StringList anymore then?
LGTM for my comments. Please don't push until JF get a look. https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/353553004/diff/20001/src/IceOperand.h#newcode126 src/IceOperand.h:126: virtual void emit(GlobalContext *Ctx) const { (void)Ctx; } On 2014/06/27 21:09:19, wala wrote: > On 2014/06/27 18:30:16, stichnot wrote: > > Assuming this is just a stub that the target lowering is expected to > > reimplement, can we just declare and not define the method, so that a missing > > target definition causes a link error? Or at least, make the implementation a > > call to llvm_unreachable(). > > Just declaring it works. There is enough variation in what the emit method needs > to do that there isn't much sense in a default implementation of emit(). I'll > also leave a comment that the target needs to specialize the emit() method for > each particular constant value, as I was unsure at first about why there was a > specialization implemented in TargetLoweringX8632.cpp. Thanks. I think it was the original way before I moved some of the emit stuff into the target lowering, and I didn't come back to clean it up. https://codereview.chromium.org/353553004/diff/40001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/353553004/diff/40001/src/IceInstX8632.cpp#new... src/IceInstX8632.cpp:230: // TODO: The isRedundantAssign() implementations for InstX8632Mov, TODO(stichnot)
I've implemented both your suggestions in the latest patch set. I also noticed a small mistake in the RUN line for one of the tests. The test vector-const.ll should only output ICE instructions, so I changed all invocations of llvm2ice with llvm2iceinsts. I hope that's okay? I'll wait until JF has a look before committing.
https://codereview.chromium.org/353553004/diff/100001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/100001/src/IceDefs.h#newcode71 src/IceDefs.h:71: typedef llvm::SmallVector<char, VECT128_BYTES> Vect128; You should use uint8_t here. I'd also name the class Vect128Bit or something similar, it gets confusing to have numbers without a unit otherwise. https://codereview.chromium.org/353553004/diff/100001/src/IceDefs.h#newcode72 src/IceDefs.h:72: typedef llvm::SmallVector<bool, VECT128_BYTES> BitVect; Can you add a comment that explains this type? https://codereview.chromium.org/353553004/diff/100001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/353553004/diff/100001/src/IceGlobalContext.cp... src/IceGlobalContext.cpp:220: } You should never have constant <? x i1> bit vectors loaded or stored: the PNaCl ABI doesn't allow it. I think this should be an error condition. https://codereview.chromium.org/353553004/diff/100001/src/IceGlobalContext.cp... src/IceGlobalContext.cpp:280: return ConstPool->BitVectors.getConstantPool(); Same comment, this shouldn't happen. https://codereview.chromium.org/353553004/diff/100001/src/llvm2ice.cpp File src/llvm2ice.cpp (right): https://codereview.chromium.org/353553004/diff/100001/src/llvm2ice.cpp#newcod... src/llvm2ice.cpp:126: // in PNaCl IR. Why handle these if they never occur, instead of asserting? https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/vector-const.ll (right): https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/vector-const.ll:158: extremes: It's not immediately obvious to me: are you testing with a few NaN patterns (signed/unsigned, signaling/not, with bits and all zeroes) as well as with denormals? The extremes section has some, but it doesn't seem thorough.
I've made some major changes to the patch (in particular, I've decided to remove support for vector constants) and I'd like everyone to take a look at it again. The description has been updated accordingly. The patch is much smaller now. I won't commit unless I get new LGTMs. JF: thanks for looking at it. I've responded to your comments. https://codereview.chromium.org/353553004/diff/100001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/353553004/diff/100001/src/IceDefs.h#newcode71 src/IceDefs.h:71: typedef llvm::SmallVector<char, VECT128_BYTES> Vect128; On 2014/06/30 17:48:50, JF wrote: > You should use uint8_t here. I'd also name the class Vect128Bit or something > similar, it gets confusing to have numbers without a unit otherwise. Unless vector constants are supported, this type is unnecessary. I'll keep your naming suggestion in mind for the future. https://codereview.chromium.org/353553004/diff/100001/src/IceDefs.h#newcode72 src/IceDefs.h:72: typedef llvm::SmallVector<bool, VECT128_BYTES> BitVect; On 2014/06/30 17:48:50, JF wrote: > Can you add a comment that explains this type? Unless vector constants are supported, this type is unnecessary. https://codereview.chromium.org/353553004/diff/100001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/353553004/diff/100001/src/IceGlobalContext.cp... src/IceGlobalContext.cpp:220: } On 2014/06/30 17:48:50, JF wrote: > You should never have constant <? x i1> bit vectors loaded or stored: the PNaCl > ABI doesn't allow it. I think this should be an error condition. See comment in llvm2ice.cpp. In addition, undefs are represented as zeros which by default are pooled. There is way to avoid this for vector types so that we do not need constant pools that hold zero vectors. https://codereview.chromium.org/353553004/diff/100001/src/IceGlobalContext.cp... src/IceGlobalContext.cpp:280: return ConstPool->BitVectors.getConstantPool(); On 2014/06/30 17:48:50, JF wrote: > Same comment, this shouldn't happen. Done. https://codereview.chromium.org/353553004/diff/100001/src/llvm2ice.cpp File src/llvm2ice.cpp (right): https://codereview.chromium.org/353553004/diff/100001/src/llvm2ice.cpp#newcod... src/llvm2ice.cpp:126: // in PNaCl IR. On 2014/06/30 17:48:50, JF wrote: > Why handle these if they never occur, instead of asserting? This was useful to me to test parts of my implementation, but it isn't necessary for PNaCl support. I can remove it. https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/vector-const.ll (right): https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/vector-const.ll:158: extremes: On 2014/06/30 17:48:50, JF wrote: > It's not immediately obvious to me: are you testing with a few NaN patterns > (signed/unsigned, signaling/not, with bits and all zeroes) as well as with > denormals? The extremes section has some, but it doesn't seem thorough. I did not think very thorough tests were necessary as the vector internally represented all values as strings of raw data, so that the underlying numbers shouldn't matter too much. Unless vector constants are supported, this test is unnecessary.
https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/vector-const.ll (right): https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/vector-const.ll:158: extremes: On 2014/06/30 22:13:24, wala wrote: > On 2014/06/30 17:48:50, JF wrote: > > It's not immediately obvious to me: are you testing with a few NaN patterns > > (signed/unsigned, signaling/not, with bits and all zeroes) as well as with > > denormals? The extremes section has some, but it doesn't seem thorough. > > I did not think very thorough tests were necessary as the vector internally > represented all values as strings of raw data, so that the underlying numbers > shouldn't matter too much. > > Unless vector constants are supported, this test is unnecessary. That's a bit too "whitebox testing" IMO: you shouldn't assume that the implementation behaves as expected w.r.t. NaN and denormals because those are a source of frequent bugs. It would be useful to do constant rematerialization at some point (if only to do better instruction selection), so I think a test along these lines will still be required. https://codereview.chromium.org/353553004/diff/120001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/120001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1317: case IceType_v4f32: I'm not sure I understand correctly, but this seems wrong: calls can return vectors.
https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/vector-const.ll (right): https://codereview.chromium.org/353553004/diff/100001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/vector-const.ll:158: extremes: On 2014/06/30 22:24:04, JF wrote: > On 2014/06/30 22:13:24, wala wrote: > > On 2014/06/30 17:48:50, JF wrote: > > > It's not immediately obvious to me: are you testing with a few NaN patterns > > > (signed/unsigned, signaling/not, with bits and all zeroes) as well as with > > > denormals? The extremes section has some, but it doesn't seem thorough. > > > > I did not think very thorough tests were necessary as the vector internally > > represented all values as strings of raw data, so that the underlying numbers > > shouldn't matter too much. > > > > Unless vector constants are supported, this test is unnecessary. > > That's a bit too "whitebox testing" IMO: you shouldn't assume that the > implementation behaves as expected w.r.t. NaN and denormals because those are a > source of frequent bugs. > > It would be useful to do constant rematerialization at some point (if only to do > better instruction selection), so I think a test along these lines will still be > required. Okay, I will add such a test at the point that there is enough support for rematerializing constants. https://codereview.chromium.org/353553004/diff/120001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/120001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1317: case IceType_v4f32: On 2014/06/30 22:24:04, JF wrote: > I'm not sure I understand correctly, but this seems wrong: calls can return > vectors. This will be implemented, along with the rest of the support necessary for passing and returning values of vector type, in a later CL. I'll add a TODO.
https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... src/IceTargetLoweringX8632.cpp:91: llvm::raw_string_ostream BaseOS(Result); On 2014/06/27 21:09:19, wala wrote: > On 2014/06/27 18:30:16, stichnot wrote: > > I'd like to avoid unnecessarily building LLVM stream stuff into the Subzero > > core. I realize the ickiness of using snprintf to render strings from > integers, > > as is done elsewhere in Subzero. In this case, you might be able to do > > something with xargs, using #elts to stringize the integer. > > To avoid snprintf, this function could emit the value directly into an Ostream > that it has been passed. I find this statement about not using raw_string_ostream perplexing, since IceConverter.cpp uses this in a lot of places. Hence, the code will be drawn in!
On 2014/07/01 20:31:55, Karl wrote: > https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... > File src/IceTargetLoweringX8632.cpp (right): > > https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... > src/IceTargetLoweringX8632.cpp:91: llvm::raw_string_ostream BaseOS(Result); > On 2014/06/27 21:09:19, wala wrote: > > On 2014/06/27 18:30:16, stichnot wrote: > > > I'd like to avoid unnecessarily building LLVM stream stuff into the Subzero > > > core. I realize the ickiness of using snprintf to render strings from > > integers, > > > as is done elsewhere in Subzero. In this case, you might be able to do > > > something with xargs, using #elts to stringize the integer. > > > > To avoid snprintf, this function could emit the value directly into an Ostream > > that it has been passed. > > I find this statement about not using raw_string_ostream perplexing, since > IceConverter.cpp uses this in a lot of places. Hence, the code will be drawn in! After discussing with Karl, I retract my comment about not pulling in LLVM's ostream stuff.
Still LGTM. https://codereview.chromium.org/353553004/diff/140001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/353553004/diff/140001/src/IceGlobalContext.cp... src/IceGlobalContext.cpp:242: llvm_unreachable("Unsupported type"); Add the type name to the error message (here and below). https://codereview.chromium.org/353553004/diff/140001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/140001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1339: llvm_unreachable("Unhandled dest type"); Add the type name to the error message. https://codereview.chromium.org/353553004/diff/140001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/353553004/diff/140001/src/IceTypes.cpp#newcode68 src/IceTypes.cpp:68: llvm_unreachable("Invalid type for typeNumElements()"); Add the type name to the error message (here and below).
https://codereview.chromium.org/353553004/diff/140001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/353553004/diff/140001/src/IceGlobalContext.cp... src/IceGlobalContext.cpp:242: llvm_unreachable("Unsupported type"); On 2014/07/07 17:47:23, stichnot wrote: > Add the type name to the error message (here and below). Done. https://codereview.chromium.org/353553004/diff/140001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/140001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1339: llvm_unreachable("Unhandled dest type"); On 2014/07/07 17:47:23, stichnot wrote: > Add the type name to the error message. Done, as a Cfg error. https://codereview.chromium.org/353553004/diff/140001/src/IceTypes.cpp File src/IceTypes.cpp (right): https://codereview.chromium.org/353553004/diff/140001/src/IceTypes.cpp#newcode68 src/IceTypes.cpp:68: llvm_unreachable("Invalid type for typeNumElements()"); On 2014/07/07 17:47:23, stichnot wrote: > Add the type name to the error message (here and below). There is no type name. This line is only triggered if the index to the type table is out of bounds.
one note -- otherwise LGTM https://codereview.chromium.org/353553004/diff/180001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/180001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:203: VectorRegisters[val] = isFP; \ Is there any reason to have a separate VectorRegisters bitvector vs FloatRegisters? Is it mostly for clarity?
https://codereview.chromium.org/353553004/diff/180001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/353553004/diff/180001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:203: VectorRegisters[val] = isFP; \ On 2014/07/07 22:48:51, jvoung wrote: > Is there any reason to have a separate VectorRegisters bitvector vs > FloatRegisters? Is it mostly for clarity? It's for clarity.
Message was sent while issue was closed.
Committed patchset #7 manually as r928f129 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
