[PATCH] [RFC]TILE-Gx: updated patch for final review
Sean Silva
silvas at purdue.edu
Tue Mar 26 07:10:57 PDT 2013
Thanks for the quick response :)
Here are a couple more small easy-to-fix issues.
================
Comment at: lib/CodeGen/TargetInfo.cpp:4829-4831
@@ +4828,5 @@
+ SmallVector<llvm::Type*, 10> &ArgList) const;
+ llvm::Type* HandleAggregates(QualType Ty, uint64_t TySize) const;
+ llvm::Type* getPaddingType(uint64_t Align, uint64_t Offset) const;
+ llvm::Type* returnAggregateInRegs(QualType RetTy, uint64_t Size) const;
+public:
----------------
`*` goes on the right. Here and in other places. (and `&` too)
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:278
@@ +277,3 @@
+
+#include <stdio.h>
+SDValue
----------------
Is this a leftover from debugging? Please remove it.
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:397-400
@@ +396,6 @@
+ // PIC
+ unsigned char flag1 = TileII::MO_HW1_LAST_GOT;
+ unsigned char flag2 = TileII::MO_HW0_GOT;
+ SDValue BaseReg = GetGlobalReg(DAG, MVT::i64);
+ bool internal =
+ GV->hasInternalLinkage() || (GV->hasLocalLinkage() && !isa<Function>(GV));
----------------
`flag1`, `flag2`, and `internal` should be capitalized.
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:490-492
@@ +489,5 @@
+ SDValue Ret = CallResult.first;
+ Ret = DAG.getNode(TileISD::TLSRELAXADD1, dl, PtrVT, Ret, TA_RELAX_1);
+ return Ret;
+ } else if (model == TLSModel::InitialExec) {
+ SDValue TAH = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
----------------
Don't use else after return. <http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return>
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:538-539
@@ +537,4 @@
+ JTI = DAG.getTargetJumpTable(JT->getIndex(), PtrVT, TileII::MO_HW0);
+ return DAG.getNode(TileISD::SAR16, dl, PtrVT, TMP, JTI);
+ } else {
+ JTI =
----------------
<http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return>
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:860-861
@@ +859,4 @@
+ << EVT(ArgVT).getEVTString();
+#endif
+ llvm_unreachable(0);
+ }
----------------
The argument to llvm_unreachable should be an explanatory string.
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:848-849
@@ +847,4 @@
+ CCState &CCInfo, const SmallVectorImpl<ISD::OutputArg> &Outs) {
+ unsigned NumOps = Outs.size();
+ for (unsigned i = 0; i != NumOps; ++i) {
+ MVT ArgVT = Outs[i].VT;
----------------
Is there a particular reason that you assign this to NumOps instead of `for (unsigned i = 0, e = Outs.size();`?
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:888-889
@@ +887,4 @@
+ const uint16_t *Reg =
+ std::find(TileIntRegs, TileIntRegs + 10, VA.getLocReg());
+ const uint16_t *RegEnd = TileIntRegs + 10;
+
----------------
I'm not familiar with the Tile architecture so maybe it is obvious and I'm just ignorant, but could you give this magic number 10 a name?
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:884-888
@@ +883,7 @@
+
+ if (!IsRegLoc)
+ LocMemOffset = VA.getLocMemOffset();
+ else {
+ const uint16_t *Reg =
+ std::find(TileIntRegs, TileIntRegs + 10, VA.getLocReg());
+ const uint16_t *RegEnd = TileIntRegs + 10;
----------------
Is there some way for you to factor this function so that you can take an early exit here? Or move this huge `else` block into another function?
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:871-876
@@ +870,8 @@
+// Copy Tile byVal arg to registers and stack.
+void static PassByValArg64(
+ SDValue &ByValChain, SDValue Chain, DebugLoc dl,
+ SmallVector<std::pair<unsigned, SDValue>, 10> &RegsToPass,
+ SmallVector<SDValue, 8> &MemOpChains, int &LastFI, MachineFrameInfo *MFI,
+ SelectionDAG &DAG, SDValue Arg, const CCValAssign &VA,
+ const ISD::ArgFlagsTy &Flags, EVT PtrTy, bool isLittle) {
+ unsigned ByValSize = Flags.getByValSize();
----------------
This is a ridiculous number of arguments! Is there some way to simplify this? This function is generally huge and it would be nice if you could break it up.
Also, please follow our naming convention and make the first letter lower-case.
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:1029-1036
@@ +1028,10 @@
+ case CCValAssign::Full:
+ if (VA.isRegLoc()) {
+ if ((ValVT == MVT::f32 && LocVT == MVT::i32) ||
+ (ValVT == MVT::f64 && LocVT == MVT::i64))
+ Arg = DAG.getNode(ISD::BITCAST, dl, LocVT, Arg);
+ else if (ValVT == MVT::f64 && LocVT == MVT::i32)
+ llvm_unreachable("No support for this yet!\n");
+ }
+ break;
+ case CCValAssign::SExt:
----------------
Break this up as:
```
if (!VA.isRegLoc())
break;
if ((ValVT == MVT::f32 && LocVT == MVT::i32) ||
(ValVT == MVT::f64 && LocVT == MVT::i64)) {
Arg = DAG.getNode(ISD::BITCAST, dl, LocVT, Arg);
break;
}
...
```
================
Comment at: lib/Target/Tile/TileISelLowering.cpp:1391
@@ +1390,3 @@
+ // Copy the result values into the output registers.
+ for (unsigned i = 0; i != RVLocs.size(); ++i) {
+ CCValAssign &VA = RVLocs[i];
----------------
`e = RVLocs.size()`;
================
Comment at: lib/Target/Tile/TileInstrInfo.cpp:175-177
@@ +174,5 @@
+
+ // Skip all the debug instructions.
+ while (I != REnd && I->isDebugValue())
+ ++I;
+
----------------
This loop appears in more than one place. Please factor it out into a function `skipDebugInstructions`
================
Comment at: lib/Target/Tile/TileInstrInfo.cpp:254
@@ +253,3 @@
+
+ for (unsigned i = 1; i < Cond.size(); ++i)
+ MIB.addReg(Cond[i].getReg());
----------------
`e = Cond.size()`
================
Comment at: lib/Target/Tile/TileMCInstLower.cpp:126-128
@@ +125,5 @@
+
+static unsigned MapToOpcodeWithIssueSlot(
+ unsigned op, const unsigned issue_slot,
+ const TileII::TileIssueType issue_type) {
+
----------------
Please follow our naming convention. (function name and argument names)
================
Comment at: lib/Target/Tile/TileRegisterInfo.cpp:114-117
@@ +113,6 @@
+
+ if (CSI.size()) {
+ MinCSFI = CSI[0].getFrameIdx();
+ MaxCSFI = CSI[CSI.size() - 1].getFrameIdx();
+ }
+
----------------
Can you just use `CSI.front()` and `CSI.back()` instead of indexing?
================
Comment at: lib/Target/Tile/TileRegisterInfo.cpp:196-202
@@ +195,8 @@
+
+unsigned TileRegisterInfo::getEHExceptionRegister() const {
+ llvm_unreachable("What is the exception register");
+}
+
+unsigned TileRegisterInfo::getEHHandlerRegister() const {
+ llvm_unreachable("What is the exception handler register");
+}
----------------
These messages are confusing. They shouldn't be asking a question; they should be explaining why they are unreachable.
================
Comment at: lib/Target/Tile/TileVLIWPacketizer.cpp:251-265
@@ +250,17 @@
+ // zero-reg can be targeted by multiple instructions.
+ FoundSequentialDependence = true;
+ } else if (DepType == SDep::Order && Dep.isArtificial()) {
+ // Ignore artificial dependencies.
+ // do nothing
+ } else if (DepType != SDep::Anti) {
+ // Skip over anti-dependences. Two instructions that are
+ // anti-dependent can share a packet.
+ FoundSequentialDependence = true;
+ }
+ }
+
+ if (FoundSequentialDependence) {
+ Dependence = true;
+ return false;
+ }
+
----------------
Instead of assigning to `FoundSequentialDependence`, can you use early exits here? That would also simplify the condition in the for loop.
================
Comment at: test/CodeGen/Tile/ctpop.ll:12-14
@@ +11,5 @@
+
+define i32 @test32(i32 %x) {
+; CHECK: v4int_l [[REG:r[0-9]+]], zero, r{{[0-9]+}}
+; CHECK: pcnt r{{[0-9]+}}, [[REG]]
+ %count = tail call i32 @llvm.ctpop.i32(i32 %x)
----------------
Please check for the name of the function to make sure that you are testing the code generated for this function. This makes the tests more robust. (I see that you are doing this in some of the tests; please do it in all of the tests). Also, please put the `; CHECK funcname:` as the first thing in the function (before `entry:`), to make it clear that it is just checking the function name (this is how the AArch64 tests do it too).
================
Comment at: test/CodeGen/Tile/fmul.ll:11-15
@@ +10,7 @@
+
+define float @test1(float %a) nounwind {
+ %ret = fmul float %a, 2.0
+ ret float %ret
+; CHECK: test0
+; CHECK: __addsf3
+}
----------------
`; CHECK: test0` inside `@test1` seems like a bug.
http://llvm-reviews.chandlerc.com/D573
More information about the llvm-commits
mailing list