Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380023002:
[SubZero] Vector types support for MIPS (Closed)
|
Created:
4 years, 2 months ago by jaydeep.patil Modified:
4 years, 2 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com, simon.dardis_imgtec.com, matthew.fortune_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Description[SubZero] Vector types support for MIPS
This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32).
Vector operations are scalarized prior to lowering. Each vector variable is split into 4 containers to hold a variable of vector type.
For MIPS32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable.
Lit test vector-mips.ll has been added to test this implementation.
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=958ddb75696187bd45682d464b5b2eb8ca378e06
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed review comments #
Total comments: 38
Patch Set 3 : Addressed review comments #
Total comments: 2
Messages
Total messages: 20 (5 generated)
Description was changed from ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is spilt into 4 containers to hold a variable of vector type. For MISP32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. ========== to ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is spilt into 4 containers to hold a variable of vector type. For MISP32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. ==========
Description was changed from ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is spilt into 4 containers to hold a variable of vector type. For MISP32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. ========== to ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is split into 4 containers to hold a variable of vector type. For MIPS32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. ==========
Initial comments. I haven't looked at the big file yet. https://codereview.chromium.org/2380023002/diff/1/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/2380023002/diff/1/src/IceCfg.cpp#newcode123 src/IceCfg.cpp:123: if (Target->shouldSplitToVariableVecOn32(Ty)) { Probably better (more regular) like this: if (Target->shouldSplitToVariableVecOn32(Ty)) { Var = VariableVecOn32::create(this, Ty, Index); } else if (Target->shouldSplitToVariable64On32(Ty)) { Var = Variable64On32::create(this, Ty, Index); } else { Var = Variable::create(this, Ty, Index); } https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode966 src/IceOperand.h:966: // VariableVecOn32 represents a vectory variable on a 32-bit architecture. In s/vectory/vector/ https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode981 src/IceOperand.h:981: if (Containers.size()) { !Containers.empty() here and below. empty() should be guaranteed to be at least as fast as (size()==0) even if you change the underlying type of Containers. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode982 src/IceOperand.h:982: for (size_t I = 0; I < NumContainers; I++) { We usually prefer pre-incement (++I), as ugly as that is, just to make it less likely to use the possibly less efficient post-increment for non-primitive types. Also, for simple one-character index variables like this, feel free to use lowercase "i", "j", etc. Technically it violates LLVM coding standards, but LLVM code itself is riddled with this exception. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode991 src/IceOperand.h:991: for (size_t I = 0; I < NumContainers; I++) { for (Variable &Var : Containers) { Var.setName(...); } https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode1013 src/IceOperand.h:1013: size_t getNumContainers() { return NumContainers; } mark all these getters as const https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode1046 src/IceOperand.h:1046: size_t NumElements = 0; In the interest of keeping this data structure small, is it possible to derive NumElements and ElementType based on Operand::Ty? https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode1050 src/IceOperand.h:1050: size_t NumContainers; Can you remove this field, since it is redundant with Containers.size() ? https://codereview.chromium.org/2380023002/diff/1/src/IceTargetLowering.h File src/IceTargetLowering.h (right): https://codereview.chromium.org/2380023002/diff/1/src/IceTargetLowering.h#new... src/IceTargetLowering.h:260: virtual bool shouldSplitToVariableVecOn32(Type Ty) const = 0; Since this is likely to be MIPS-specific, maybe provide a default "return false;" implementation, instead of making it pure virtual? That way, the non-MIPS targets won't need to be touched.
https://codereview.chromium.org/2380023002/diff/1/src/IceCfg.cpp File src/IceCfg.cpp (right): https://codereview.chromium.org/2380023002/diff/1/src/IceCfg.cpp#newcode123 src/IceCfg.cpp:123: if (Target->shouldSplitToVariableVecOn32(Ty)) { On 2016/09/29 16:50:25, stichnot wrote: > Probably better (more regular) like this: > > if (Target->shouldSplitToVariableVecOn32(Ty)) { > Var = VariableVecOn32::create(this, Ty, Index); > } else if (Target->shouldSplitToVariable64On32(Ty)) { > Var = Variable64On32::create(this, Ty, Index); > } else { > Var = Variable::create(this, Ty, Index); > } Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode966 src/IceOperand.h:966: // VariableVecOn32 represents a vectory variable on a 32-bit architecture. In On 2016/09/29 16:50:25, stichnot wrote: > s/vectory/vector/ Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode981 src/IceOperand.h:981: if (Containers.size()) { On 2016/09/29 16:50:25, stichnot wrote: > !Containers.empty() > > here and below. > > empty() should be guaranteed to be at least as fast as (size()==0) even if you > change the underlying type of Containers. Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode982 src/IceOperand.h:982: for (size_t I = 0; I < NumContainers; I++) { On 2016/09/29 16:50:25, stichnot wrote: > We usually prefer pre-incement (++I), as ugly as that is, just to make it less > likely to use the possibly less efficient post-increment for non-primitive > types. > > Also, for simple one-character index variables like this, feel free to use > lowercase "i", "j", etc. Technically it violates LLVM coding standards, but > LLVM code itself is riddled with this exception. Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode991 src/IceOperand.h:991: for (size_t I = 0; I < NumContainers; I++) { On 2016/09/29 16:50:25, stichnot wrote: > for (Variable &Var : Containers) { > Var.setName(...); > } Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode1013 src/IceOperand.h:1013: size_t getNumContainers() { return NumContainers; } On 2016/09/29 16:50:25, stichnot wrote: > mark all these getters as const Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode1046 src/IceOperand.h:1046: size_t NumElements = 0; On 2016/09/29 16:50:25, stichnot wrote: > In the interest of keeping this data structure small, is it possible to derive > NumElements and ElementType based on Operand::Ty? Done. https://codereview.chromium.org/2380023002/diff/1/src/IceOperand.h#newcode1050 src/IceOperand.h:1050: size_t NumContainers; On 2016/09/29 16:50:25, stichnot wrote: > Can you remove this field, since it is redundant with Containers.size() ? Done. https://codereview.chromium.org/2380023002/diff/1/src/IceTargetLowering.h File src/IceTargetLowering.h (right): https://codereview.chromium.org/2380023002/diff/1/src/IceTargetLowering.h#new... src/IceTargetLowering.h:260: virtual bool shouldSplitToVariableVecOn32(Type Ty) const = 0; On 2016/09/29 16:50:25, stichnot wrote: > Since this is likely to be MIPS-specific, maybe provide a default "return > false;" implementation, instead of making it pure virtual? > > That way, the non-MIPS targets won't need to be touched. Done.
A few more comments apart from the lowering file. One question in the meantime. Is this meant to be the actual way vector operations are lowered, or just an interim solution to get tests running? In either case, you'll want to make sure calling conventions match LLVM, specifically args-in-registers, stack slot padding for alignment, and overall expected stack alignment. This is so that cross tests and bisection debugging can work. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:966: // VariableVecOn32 represents a vector variable on a 32-bit architecture. In How about: ... represents a 128-bit vector variable ... so that the number 4 is more clear https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:972: In this private section, could you define a static constexpr variable describing the number of container elements per vector value? e.g. // A 128-bit vector value is mapped onto 4 32-bit register values. static constexpr SizeT ElementsPerContainer = 4; https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:981: if (!Containers.empty()) { I think you can remove the .empty() test. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:990: if (!Containers.empty()) { Definitely remove the .empty() test here. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:1001: for (SizeT i = 0; i < 4; ++i) { ... i < ElementsPerContainer; ... https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:1019: assert(typeWidthInBytes(Ty) == 16); Instead of "16", maybe ElementsPerContainer * typeWidthInBytes(IceType_i32) https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:1023: Type VectorType; It doesn't look like this field is actually used anywhere. Can you remove it?
I really haven't looked that deeply into the actual vector lowering sequences. I'll have much higher confidence once the cross tests are running. The main thing that cross tests might not catch would be if there's something weird and incorrect about the lowering that doesn't trigger a liveness validation failure but manifests itself in the register allocator on larger functions. https://codereview.chromium.org/2380023002/diff/20001/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceCfg.h#newcode332 src/IceCfg.h:332: Variable *ImplicitRet; /// Implicit return This is something that, so far, is only needed by MIPS. What do you think about moving it and its getter/setter into TargetMIPS32? https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:972: On 2016/09/30 15:41:59, stichnot wrote: > In this private section, could you define a static constexpr variable describing > the number of container elements per vector value? e.g. > > // A 128-bit vector value is mapped onto 4 32-bit register values. > static constexpr SizeT ElementsPerContainer = 4; Actually, maybe it would be better to put this constexpr into the public section, so that many/most of the uses of "4" in the target lowering code could use that constexpr instead. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:417: if (Dest && isVectorType(Dest->getType()) && ID == Intrinsics::Fabs) { Can probably just assert(Dest != nullptr) https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1414: ArgNo++; ++ArgNo for default pre-increment consistency? https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1724: if (auto *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { llvm::isa<> otherwise warning/error for unused Const Also, is it appropriate to report an error for a Constant in general, and not just ConstantInteger64? Actually, looking even further down, if you didn't test this at all, wouldn't it just fall through to the "Unsupported operand type" error at the end? https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1736: constexpr bool SignExt = false; A false constexpr value is usually "documented" something like this: constexpr bool NoSignExt = false; ... canHoldOffset(NoSignExt) ... https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1738: Constant *Four = Ctx->getConstantInt32(4); With this pattern, we would usually name the variable "_4" Constant *_4 = ...(4); though I see the ARM lowering still has one instance of "Four"... https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2191: _mov(DestHi, T_Hi); Can you restructure this like the following? if (DestTy == IceType_i64) { ... return; } if (isVectorType(DestTy)) { ... return; } ... // other cases https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2360: ArgNum++; ++ArgNum https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2746: Type DestTy = Dest->getType(); const Type https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:3116: Type DestTy = Dest->getType(); const Type https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:3695: break; Maybe there should still be a hard error here? (just no longer UnimplementedLoweringError)
On 2016/09/30 17:37:40, stichnot wrote: > I really haven't looked that deeply into the actual vector lowering sequences. > I'll have much higher confidence once the cross tests are running. The main > thing that cross tests might not catch would be if there's something weird and > incorrect about the lowering that doesn't trigger a liveness validation failure > but manifests itself in the register allocator on larger functions. I have tried running test_fcmp cross test but found an issue with the calling convention (Clang vs pnacl-llc). In pnacl-llc, the <4 x float> type is treated differently than its C version v4f32. We use <4 x float> type in test_fcmp_pnacl.ll and v4f32 in its driver .cpp file. The cross test framework assumes that these two types are same. However pnacl-llc treats <4 x float> as array of float type and uses MIPS FP registers $f12,$f14 etc. for arguments. We are using v4f32 type in driver .cpp file to call fcmpXXXVector function defined in .ll file. The driver .cpp file is compiled using Clang. Clang v4f32 treats it as vector of 4 elements and uses MIPS GP registers $4-$7 to call fcmpXXXVector. Thus $f12,$f14 are never setup by the caller, and the callee uses them without actual def. I will talk to the MIPS LLVM compiler and get some clarification on this. Once the support for vector type and cast is available then it will be very easy to run the cross tests.
On 2016/09/30 15:41:59, stichnot wrote: > A few more comments apart from the lowering file. > > One question in the meantime. Is this meant to be the actual way vector > operations are lowered Yes, this is how code is generated by Clang/GCC. >, or just an interim solution to get tests running? No this is not an interim solution. > In either case, you'll want to make sure calling conventions match LLVM, > specifically args-in-registers, stack slot padding for alignment, and overall > expected stack alignment. This is so that cross tests and bisection debugging > can work. > Vectors are passed as if they are four i32 variables. Calling convention rules used for i32/i64 are applied here as well.
https://codereview.chromium.org/2380023002/diff/20001/src/IceCfg.h File src/IceCfg.h (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceCfg.h#newcode332 src/IceCfg.h:332: Variable *ImplicitRet; /// Implicit return On 2016/09/30 17:37:39, Jim - vacation through Oct. 4 wrote: > This is something that, so far, is only needed by MIPS. What do you think about > moving it and its getter/setter into TargetMIPS32? Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:966: // VariableVecOn32 represents a vector variable on a 32-bit architecture. In On 2016/09/30 15:41:59, Jim - vacation through Oct. 4 wrote: > How about: > ... represents a 128-bit vector variable ... > > so that the number 4 is more clear Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:972: On 2016/09/30 15:41:59, Jim - vacation through Oct. 4 wrote: > In this private section, could you define a static constexpr variable describing > the number of container elements per vector value? e.g. > > // A 128-bit vector value is mapped onto 4 32-bit register values. > static constexpr SizeT ElementsPerContainer = 4; Acknowledged. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:972: On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > On 2016/09/30 15:41:59, stichnot wrote: > > In this private section, could you define a static constexpr variable > describing > > the number of container elements per vector value? e.g. > > > > // A 128-bit vector value is mapped onto 4 32-bit register values. > > static constexpr SizeT ElementsPerContainer = 4; > > Actually, maybe it would be better to put this constexpr into the public > section, so that many/most of the uses of "4" in the target lowering code could > use that constexpr instead. Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:981: if (!Containers.empty()) { On 2016/09/30 15:41:58, Jim - vacation through Oct. 4 wrote: > I think you can remove the .empty() test. We need this check here. We may not have called initVecElement (like initHiLo for instance). https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:990: if (!Containers.empty()) { On 2016/09/30 15:41:59, Jim - vacation through Oct. 4 wrote: > Definitely remove the .empty() test here. Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:1001: for (SizeT i = 0; i < 4; ++i) { On 2016/09/30 15:41:58, Jim - vacation through Oct. 4 wrote: > ... i < ElementsPerContainer; ... Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:1019: assert(typeWidthInBytes(Ty) == 16); On 2016/09/30 15:41:59, Jim - vacation through Oct. 4 wrote: > Instead of "16", maybe > ElementsPerContainer * typeWidthInBytes(IceType_i32) Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:1023: Type VectorType; On 2016/09/30 15:41:59, Jim - vacation through Oct. 4 wrote: > It doesn't look like this field is actually used anywhere. Can you remove it? Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:417: if (Dest && isVectorType(Dest->getType()) && ID == Intrinsics::Fabs) { On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > Can probably just assert(Dest != nullptr) We need to check for Dest != nullptr here (instead of an assert macro) as Dest could be null for calls other than llvm.fabs.v4f32 (for example declare void @llvm.nacl.longjmp(i8*, i32)). https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1414: ArgNo++; On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > ++ArgNo for default pre-increment consistency? Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1724: if (auto *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > llvm::isa<> > > otherwise warning/error for unused Const > > Also, is it appropriate to report an error for a Constant in general, and not > just ConstantInteger64? > > Actually, looking even further down, if you didn't test this at all, wouldn't it > just fall through to the "Unsupported operand type" error at the end? Yes, not tested for Constants. We need to introduce ConstantInteger128 for cases like "v4i32 Var = {1,2,3,4};" Removing this check for now. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1736: constexpr bool SignExt = false; On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > A false constexpr value is usually "documented" something like this: > > constexpr bool NoSignExt = false; > ... canHoldOffset(NoSignExt) ... Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1738: Constant *Four = Ctx->getConstantInt32(4); On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > With this pattern, we would usually name the variable "_4" > Constant *_4 = ...(4); > though I see the ARM lowering still has one instance of "Four"... Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2191: _mov(DestHi, T_Hi); On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > Can you restructure this like the following? > > if (DestTy == IceType_i64) { > ... > return; > } > if (isVectorType(DestTy)) { > ... > return; > } > ... // other cases Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2360: ArgNum++; On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > ++ArgNum Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2746: Type DestTy = Dest->getType(); On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > const Type Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:3116: Type DestTy = Dest->getType(); On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > const Type Done. https://codereview.chromium.org/2380023002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:3695: break; On 2016/09/30 17:37:40, Jim - vacation through Oct. 4 wrote: > Maybe there should still be a hard error here? (just no longer > UnimplementedLoweringError) Done.
LGTM. (I went ahead and fixed the minor comments.) https://codereview.chromium.org/2380023002/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2380023002/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2400: ArgNum++; ++ArgNum https://codereview.chromium.org/2380023002/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/2380023002/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.h:754: Variable *ImplicitRet; /// Implicit return Variable *ImplicitRet = nullptr;
Description was changed from ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is split into 4 containers to hold a variable of vector type. For MIPS32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. ========== to ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is split into 4 containers to hold a variable of vector type. For MIPS32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 958ddb75696187bd45682d464b5b2eb8ca378e06 (presubmit successful).
Message was sent while issue was closed.
On 2016/10/03 14:52:52, Jim - vacation through Oct. 4 wrote: > Committed patchset #3 (id:40001) manually as > 958ddb75696187bd45682d464b5b2eb8ca378e06 (presubmit successful). Hi Jim, Thanks for the fixes and the review :) Regards, Jaydeep
Message was sent while issue was closed.
Description was changed from ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is split into 4 containers to hold a variable of vector type. For MIPS32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ========== to ========== [SubZero] Vector types support for MIPS This patch implements vector operations on MIPS32 using VariableVecOn32 method (on the lines of Variable64On32). Vector operations are scalarized prior to lowering. Each vector variable is split into 4 containers to hold a variable of vector type. For MIPS32, four GP/FP registers are used to hold a vector variable. Arguments are passed in GP registers irrespective of the type of the vector variable. Lit test vector-mips.ll has been added to test this implementation. R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
On 2016/10/01 05:43:33, jaydeep.patil wrote: > On 2016/09/30 17:37:40, stichnot wrote: > > I really haven't looked that deeply into the actual vector lowering sequences. > > > I'll have much higher confidence once the cross tests are running. The main > > thing that cross tests might not catch would be if there's something weird and > > incorrect about the lowering that doesn't trigger a liveness validation > failure > > but manifests itself in the register allocator on larger functions. > > I have tried running test_fcmp cross test but found an issue with the calling > convention (Clang vs pnacl-llc). > In pnacl-llc, the <4 x float> type is treated differently than its C version > v4f32. > We use <4 x float> type in test_fcmp_pnacl.ll and v4f32 in its driver .cpp file. > > The cross test framework assumes that these two types are same. > However pnacl-llc treats <4 x float> as array of float type and uses MIPS FP > registers $f12,$f14 etc. for arguments. > We are using v4f32 type in driver .cpp file to call fcmpXXXVector function > defined in .ll file. > The driver .cpp file is compiled using Clang. > Clang v4f32 treats it as vector of 4 elements and uses MIPS GP registers $4-$7 > to call fcmpXXXVector. > Thus $f12,$f14 are never setup by the caller, and the callee uses them without > actual def. > I will talk to the MIPS LLVM compiler and get some clarification on this. > > Once the support for vector type and cast is available then it will be very easy > to run the cross tests. Hi Jim, Adding Matthew Fortune and Simon Dardis for discussion on <4 x float> vs v4f32 type. Regards, Jaydeep
Message was sent while issue was closed.
Hi Jim,
I’m the code owner of the MIPS backend in LLVM. For MIPS, our ABI requires that
vectors
are passed in integer registers, so a C function with a signature like:
v4i32 fcmpOeq(v4f32 a, v4f32 b)
is lowered to LLVM IR by Clang with a signature like:
declare inreg { i32, i32, i32, i32 } @fcmpOeqVector(i32 inreg, i32 inreg, i32
inreg, i32 inreg, i32 inreg, i32 inreg, i32 inreg, i32 inreg)
Passing the vector argument through the integer registers. Other targets such as
X86_64, AArch64 would lower this to:
<4 x i32> define @fcmpoeq(<4 x float>, <4 x float>)
and pass the arguments through vector registers.
Currently the MIPS backend in LLVM will tolerate functions with <4 x float> but
will pass the vector as four floats according
to the ABI in use. This unintentional support however does not current work with
MSA, our latest vector instruction set.
I'm investigating what level of changes would be required to support <4 x float>
& co. directly as per our ABIs but this handling
was traditionally never required for MIPS as we've encoded the argument lowering
directly into LLVM-IR. As such, I have concerns
about the maintenance effort.
I can see from the crosstest suite that the vector operations test uses that
type signature. Is it expected all targets are to use
those tests?
Thanks,
Simon
From: jaydeep.patil@imgtec.com [mailto:jaydeep.patil@imgtec.com]
Sent: 06 October 2016 13:50
To: kschimpf@google.com; jpp@chromium.org; stichnot@chromium.org;
eholk@chromium.org; Sagar Thakur
Cc: native-client-reviews@googlegroups.com; Rich Fuhler; Simon Dardis; Matthew
Fortune
Subject: Re: [SubZero] Vector types support for MIPS (issue 2380023002 by
jaydeep.patil@imgtec.com)
On 2016/10/01 05:43:33, jaydeep.patil wrote:
> On 2016/09/30 17:37:40, stichnot wrote:
> > I really haven't looked that deeply into the actual vector lowering
sequences.
>
> > I'll have much higher confidence once the cross tests are running. The main
> > thing that cross tests might not catch would be if there's something weird
and
> > incorrect about the lowering that doesn't trigger a liveness validation
> failure
> > but manifests itself in the register allocator on larger functions.
>
> I have tried running test_fcmp cross test but found an issue with the calling
> convention (Clang vs pnacl-llc).
> In pnacl-llc, the <4 x float> type is treated differently than its C version
> v4f32.
> We use <4 x float> type in test_fcmp_pnacl.ll and v4f32 in its driver .cpp
file.
>
> The cross test framework assumes that these two types are same.
> However pnacl-llc treats <4 x float> as array of float type and uses MIPS FP
> registers $f12,$f14 etc. for arguments.
> We are using v4f32 type in driver .cpp file to call fcmpXXXVector function
> defined in .ll file.
> The driver .cpp file is compiled using Clang.
> Clang v4f32 treats it as vector of 4 elements and uses MIPS GP registers
$4-$7
> to call fcmpXXXVector.
> Thus $f12,$f14 are never setup by the caller, and the callee uses them
without
> actual def.
> I will talk to the MIPS LLVM compiler and get some clarification on this.
>
> Once the support for vector type and cast is available then it will be very
easy
> to run the cross tests.
Hi Jim,
Adding Matthew Fortune and Simon Dardis for discussion on <4 x float> vs v4f32
type.
Regards,
Jaydeep
https://codereview.chromium.org/2380023002/
--
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
