[llvm-commits] Patch: new backend for Hexagon processor
Jakob Stoklund Olesen
stoklund at 2pi.dk
Mon Nov 28 18:25:45 PST 2011
On Nov 28, 2011, at 3:14 PM, Tony Linthicum wrote:
> <hexagon.patch.bz2><llvm.patch.bz2>
Hi Tony,
I would have told you that you don't need to compress patches, but hexagon.patch compresses 10x. That's odd.
I think you need to make a pass over the whole patch and clean up missing punctuation and capitalization in comments. Some files have no comments at all. Commented-out code should be deleted.
All target-specific command line options should have help text and be named so it is obvious which target they belong to.
Some specifics:
Spurious hunk:
diff --git a/include/llvm/InitializePasses.h b/include/llvm/InitializePasses.h
index ceb1771..9edb875 100644
--- a/include/llvm/InitializePasses.h
+++ b/include/llvm/InitializePasses.h
@@ -232,7 +232,6 @@ void initializeUnreachableMachineBlockElimPass(PassRegistry&);
void initializeVerifierPass(PassRegistry&);
void initializeVirtRegMapPass(PassRegistry&);
void initializeInstSimplifierPass(PassRegistry&);
-
}
There is a lot of commented out code (using #if 0) in HexagonAsmPrinter. Please just remove that.
+bool HexagonInstrInfo::
+isU6_3Immediate(const int value) const {
+ return (0 <= value && value <= 511);
+}
Please use the isUInt<9> templates from MathExtras.h for these functions.
Same thing here:
+def s32ImmPred : PatLeaf<(i32 imm), [{
+ // immS16 predicate - True if the immediate fits in a 16-bit sign extended
+ // field.
+ int64_t v = (int64_t)N->getSExtValue();
+ return (v <= 2147483647 && v >= -2147483648);
+}]>;
+++ b/lib/Target/Hexagon/HexagonCFGOptimizer.cpp
There is not a lot of documentation in this file. Is it doing something that the target-independent code placement optimizers can't do?
+cl::opt<std::string> CFGFn("cfgfn", cl::Hidden, cl::desc(""), cl::init(""),
+ cl::ZeroOrMore);
+cl::opt<int> CFGBlock("cfgblock", cl::Hidden, cl::desc(""), cl::init(-1),
+ cl::ZeroOrMore);
Are these options leftovers from debugging? If so, just delete them. If not, please use longer option names, and add help text.
+static bool IsConditionalBranch(int Opc) {
+ return (Opc == Hexagon::JMP_Pred) || (Opc == Hexagon::JMP_PredNot)
+ || (Opc == Hexagon::JMP_PredPt) || (Opc == Hexagon::JMP_PredNotPt);
+}
+
+
+static bool IsUnconditionalJump(int Opc) {
+ return (Opc == Hexagon::JMP);
+}
This information should already be available from MCInstrDesc. See the isBranch() and isBarrier() methods.
+++ b/lib/Target/Hexagon/HexagonCallingConvLower.cpp
@@ -0,0 +1,207 @@
+//===-- llvm/CallingConvLower.cpp - Calling Conventions -------------------===//
The comment is wrong.
It is unfortunate that you had to duplicate this code. How much needs to be changed in the target-independent code for it to be useful?
--- /dev/null
+++ b/lib/Target/Hexagon/HexagonExpandPredSpillCode.cpp
@@ -0,0 +1,175 @@
+//===--- HexagonExpandPredSpillCode.cpp - Expand Predicate Spill Code ----===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
I think that is the only comment in this file. Please explain what this pass does.
It looks like you could just use a TII::expandPostRAPseudo() hook instead?
+++ b/lib/Target/Hexagon/HexagonFrameLowering.cpp
+static cl::opt<bool> DisableDeallocRet("disable-dealloc-ret", cl::Hidden,
+ cl::desc("Disable Dealloc Retrun"));
Please use some kind of indication that this is a target-specific command line option.
+ maxCallFrameSize = (maxCallFrameSize + AlignMask) & ~AlignMask;
Please use RoundUpToAlignment().
+#define ALLOCFRAME_MAX 16384
Use 'const unsigned' for constants instead of preprocessor macros.
+ if (RetOpcode == Hexagon::TCRETURNtg || RetOpcode == Hexagon::TCRETURNtext)
+ return true;
+ else
+ return false;
a.k.a. "return RetOpcode == Hexagon::TCRETURNtg || RetOpcode == Hexagon::TCRETURNtext"
+ // We can only schedule double loads if we spill contiguous callee-saved regs
+ // For instance, we cannot scheduled double-word loads if we spill r24,
+ // r26, and r27.
+ // Hexagon_TODO: We can try to double-word align odd registers for -O2 and
+ // above
+ bool ContiguousRegs = true;
+
+ for (unsigned i = 0; i < CSI.size(); ++i) {
+ unsigned Reg = CSI[i].getReg();
+
+ //
+ // Check if we can use a double-word store
+ //
+ const unsigned* SuperReg = TRI->getSuperRegisters(Reg);
If double-word loads and stores are always better, you can change your getCalleeSavedRegs() hook to return D-registers instead. The register allocator understands the aliasing.
+void HexagonFrameLowering::
+processFunctionBeforeFrameFinalized(MachineFunction &MF) const {
+ // do nothing
+}
+
+void HexagonFrameLowering::
+processFunctionBeforeCalleeSavedScan(MachineFunction &MF,
+ RegScavenger* RS) const {
+ // do nothing
+}
Just delete these empty overrides.
+++ b/lib/Target/Hexagon/HexagonFrameLowering.h
+#include "HexagonSubtarget.h"
You have both and include and a forward decl of HexagonSubtarget.
+++ b/lib/Target/Hexagon/HexagonHardwareLoops.cpp
+ void print(raw_ostream &OS, const TargetMachine *TM = 0) const {
+ if (isReg()) { OS << "%reg" << getReg(); }
Better pretty-printing of registers: OS << PrintReg(getReg());
+ BuildMI(*Preheader, InsertPos, InsertPos->getDebugLoc(),
+ TII->get(TargetOpcode::COPY), CountReg).addReg(TripCount->getReg());
+ if (TripCount->isNeg()) {
+ unsigned CountReg1 = CountReg;
+ CountReg = MF->getRegInfo().createVirtualRegister(RC);
+ BuildMI(*Preheader, InsertPos, InsertPos->getDebugLoc(),
+ TII->get(Hexagon::NEG), CountReg).addReg(CountReg1);
+ }
This doesn't look right. You never define CountReg1 anywhere. The machine code verifier should tell you.
+++ b/lib/Target/Hexagon/HexagonISelDAGToDAG.cpp
+static bool FitsS11_0(int64_t v) {
+ return (v <= 1023 && v >= -1024);
+}
Again, please use the MathExtras.h functions for this.
+ }
+ else if (LD->getValueType(0) == MVT::i64 &&
+ LD->getExtensionType() == ISD::SEXTLOAD) {
+ return SelectIndexedLoadSignExtend64(LD, Opcode, dl);
+ }
+ else { // Handle normal loads
Two things: Please put else on the same line as the closing brace, and don't use else after return/continue. This is happening in many places.
+++ b/lib/Target/Hexagon/HexagonISelLowering.cpp
+#define Hexagon_MAX_RET_SIZE 64
Please use C++ constants instead of macros.
+cl::opt<bool>
+ExpandBuiltinMemcpy("expand-memcpy", cl::init(true), cl::Hidden,
+ cl::desc("Enable memcpy/memset builtin expansion"));
+
+cl::opt<bool>
+EmitJumpTables("jump-tables", cl::init(true), cl::Hidden,
+ cl::desc("Emit jump tables"));
Please rename these options, and make them static.
+ if (Flag.getNode())
+ return DAG.getNode(HexagonISD::RET_FLAG, dl, MVT::Other, Chain, Flag);
Indentation.
+ //setOperationAction(ISD::DBG_STOPPOINT, MVT::Other, Expand);
+ //setOperationAction(ISD::DEBUG_LOC, MVT::Other, Expand);
+ //setOperationAction(ISD::DBG_LABEL, MVT::Other, Expand);
+ //setOperationAction(ISD::DECLARE, MVT::Other, Expand);
Please delete commented-out code. There is a lot of it.
+++ b/lib/Target/Hexagon/HexagonInstrInfo.cpp
+bool HexagonInstrInfo::isMoveInstr(const MachineInstr &MI,
This hook is obsolete and not called from anywhere. Make sure you always generate target-independent COPY instructions before register allocation instead.
+void HexagonInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator I, DebugLoc DL,
+ unsigned DestReg, unsigned SrcReg,
+ bool KillSrc) const {
+ // Hexagon_TODO: add code
It seems you already did...
+ if (Hexagon::IntRegsRegClass.contains(SrcReg) &&
+ Hexagon::IntRegsRegClass.contains(DestReg)) {
You can simply say contains(SrcReg, DestReg) here.
+ BuildMI(MBB, I, DL, get(Hexagon::TFR), DestReg).addReg(SrcReg);
+ return;
+ }
+ else if (Hexagon::DoubleRegsRegClass.contains(SrcReg) &&
... and don't else after return
+ else if (Hexagon::DoubleRegsRegClass.contains(DestReg) &&
+ Hexagon::IntRegsRegClass.contains(SrcReg)) {
+ // We can have an overlap between single and double reg: r1:0 = r0
This looks odd. How can you copy between registers of different sizes?
+ if (RC == Hexagon::IntRegsRegisterClass) {
+ BuildMI(MBB, I, DL, get(Hexagon::STriw))
+ .addFrameIndex(FI).addImm(0)
+ .addReg(SrcReg, getKillRegState(isKill)).addMemOperand(MMO);
+ }
Here, I recommend you use Hexagon::IntRegsRegisterClass->hasSubClassEq(RC). It has been a common bug in other targets when new sub-classes are added.
+MachineInstr *HexagonInstrInfo::foldMemoryOperandImpl(MachineFunction &MF,
+ MachineInstr* MI,
+ const SmallVectorImpl<unsigned> &Ops,
+ int FI) const {
+ // Hexagon_TODO: Implement
+ return(0);
+}
You probably don't need to implement this hook on a load/store architecture.
+unsigned HexagonInstrInfo::createVR(MachineFunction* MF, MVT VT) const {
This function is unused?
+bool HexagonInstrInfo::
+isS12_Immediate(const int value) const {
+ // predicate - True if the immediate fits is a 12-bit signed value
+ return (value <= 2047 && value >= -2048);
+}
These functions come in many copies... ;) I think one is enough.
+bool HexagonInstrInfo::isPredicable(MachineInstr *MI) const {
+ switch(Opc) {
+ case Hexagon::TFRI:
+ return isS12_Immediate(MI->getOperand(1).getImm());
Can you make use of TargetFlags to avoid these many opcode switches?
+bool HexagonInstrInfo::isPredicated(const MachineInstr *MI) const {
+ switch (MI->getOpcode()) {
Again, use MCInstrDesc or TargetFlags to avoid these large switches.
+bool HexagonInstrInfo::
+isValidOffset(const int Opcode, const int Offset) const {
+
+ // Hexagon_TODO: Ugh! hardcoding. Is there a target specific API that can be
+ // used?
+#define Hexagon_MEMW_OFFSET_MAX 4095
+#define Hexagon_MEMW_OFFSET_MIN -4096
I agree. You really need to clean up your literal predicates. They are literally all over the place.
+ if (VT == MVT::i64) {
+ return (Offset >= Hexagon_MEMD_AUTOINC_MIN &&
+ Offset <= Hexagon_MEMD_AUTOINC_MAX &&
+ (Offset & 0x7) == 0);
You have many checks like this. I think it would be fine to add isShiftedUInt<N,S> templates to MathExtras.h.
+bool HexagonInstrInfo::
+isMemOp(const MachineInstr *MI) const {
+ switch (MI->getOpcode())
+ {
Can you use MCInstrDesc::mayLoad / mayStore?
+++ b/lib/Target/Hexagon/HexagonInstrInfo.h
+/// SPII - This namespace holds all of the target specific flags that
+/// instruction info tracks.
+///
+namespace HexagonII {
+ enum {
+ Pseudo = (1<<0),
+ Load = (1<<1),
+ Store = (1<<2),
+ DelaySlot = (1<<3)
+ };
There is already target-independent flags for all of these in MCInstrDesc.
+++ b/lib/Target/Hexagon/HexagonInstrInfo.td
I like the straightforward instruction definitions in this file.
Could you place all the call-related instructions together. I think one escaped.
+++ b/lib/Target/Hexagon/HexagonIntrinsicsDerived.td
One file for a single pattern??
+++ b/lib/Target/Hexagon/HexagonOptimizeSZExtends.cpp
@@ -0,0 +1,129 @@
+//===-- HexagonOptimizeSZExtends.cpp - Identify and remove sign and -------===//
+//===-- zero extends. -------===//
Can this pass do something that lib/CodeGen/PeepholeOptimizer.cpp can't?
+++ b/lib/Target/Hexagon/HexagonRegisterInfo.cpp
+cl::opt<std::string> AllocFn("alloc-fn", cl::Hidden,
+ cl::desc(""));
!
+const unsigned* HexagonRegisterInfo::getCalleeSavedRegs(const MachineFunction
+ *MF)
+ const {
Weird whitespace. Use:
const unsigned* HexagonRegisterInfo::
getCalleeSavedRegs(const MachineFunction *MF) const {
+++ b/lib/Target/Hexagon/HexagonRegisterInfo.h
+#define HEXAGON_RESERVED_REG_1 Hexagon::R10
+#define HEXAGON_RESERVED_REG_2 Hexagon::R11
Hmm. Does that have to go in a header file? Have you looked into using the register scavenger instead of reserving registers?
+++ b/lib/Target/Hexagon/HexagonRegisterInfo.td
+// FIXME: the register order should be defined in terms of the preferred
+// allocation order...
+//
+def IntRegs : RegisterClass<"Hexagon", [i32], 32, (add (sequence "R%u", 0, 9),
+ (sequence "R%u", 12, 28),
+ R10, R11, R29, R30,
+ R31)>
You don't need to shuffle registers here just to put the callee-saved registers last in the allocation order. The register allocator does that automatically.
+// Hexagon_TODO: Do we really need to specify different classes for
+// FP and Int registers?
No, you don't. Just specify an [i32, f32] type vector.
+// Will this confuse register allocation?
No, it will work, but you aren't gaining anything. You can use multiple identical register classes if the types have different spill sizes, see the FR32, FR64, and VR128 register classes in the x86 target.
+// Subregister definitions. Adapted from X86RegisterInfo.td
+//
+//def Hexagon_subreg_loreg : PatLeaf<(i32 1)>;
+//def Hexagon_subreg_hireg : PatLeaf<(i32 2)>;
+//
+//def : SubRegSet<1, [D0, D1, D2, D3, D4, D5, D6, D7,
+// D8, D9, D10, D11, D12, D13, D14, D15],
+// [R0, R2, R4, R6, R8, R10,R12, R14,
+// R16, R18, R20, R22, R24, R26, R28, R30]>;
+//
+//
+//def : SubRegSet<2, [D0, D1, D2, D3, D4, D5, D6, D7,
+// D8, D9, D10, D11, D12, D13, D14, D15],
+// [R1, R3, R5, R7, R9, R11, R13, R15,
+// R17, R19, R21, R23, R25, R27, R29, R31]>;
This is obsolete. Just delete it.
+++ b/lib/Target/Hexagon/HexagonRemoveSZExtArgs.cpp
@@ -0,0 +1,86 @@
+//===-- HexagonRemoveExtendArgs.cpp - Remove unecessary sign extends ------===//
This comment is wrong. There is already a HexagonOptimizeSZExtends pass. Are both needed?
+++ b/lib/Target/Hexagon/HexagonSplitTFRCondSets.cpp
@@ -0,0 +1,119 @@
+//===---- HexagonSplitTFRCondSets.cpp - split TFR condsets into xfers -----===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
Missing non-trivial comments.
/jakob
More information about the llvm-commits
mailing list