[llvm-commits] PATCH: SPARC codegen fixes

[Chris Lattner]

On Aug 27, 2009, at 2:03 AM, Venkatraman Govindaraju wrote:

> Hello,
>
> This patch enables sparc back-end
> 1.  To generate pic code.
> 2.  To support the function linkage flags.
>
> The patch also
> 1. makes addcc, subcc, fcmp instructions and conditional branching
> instructions as defs and uses of ICC/FCC registers. Otherwise
> machine-sink pass sinks the subcc instructioin to successor basicblock
> and hence the loops won't execute at all.
> 2. Renames register %I6 to %FP and register %O6 to %SP. It makes
> easier to read llc output.
> 3. Sets function size in assembly.  This makes easier to debug llc
> output in gdb.
> 4. correctly aligns the functions to 4 byte not 16 byte.
>
> With this patch, we can now compile llvm-gcc in sparc-sun-solaris2.11
> target including libstdc++.

Whoa, nice!  Some minor stylistic comments:

+let Defs = [ICC] in {
+    defm SUBCC  : F3_12  <"subcc", 0b010100, SPcmpicc>;

Please indent by 2.

+  case MachineOperand::MO_Register:
+    if (TargetRegisterInfo::isPhysicalRegister(MO.getReg()))
+      operand = "%" + LowercaseString (RI.get(MO.getReg()).AsmName);
+    else

No space before arguments of a function call

+  if (LabelForBaseReg.find(operand) == LabelForBaseReg.end()) {
+    LabelForBaseReg[operand] = LabelForBaseReg.size();
+  }

Please use:
   if (!LabelForBaseReg.count(operand))
     LabelForBaseReg[operand] = LabelForBaseReg.size();

It would be even more efficient to just use LabelForBaseReg.insert 
(std::make_pair(operand, ..size))


+  // Print out PIC Base computation helpers

Please properly punctuate the sentence (end with .)


Please don't use tabs in the code.


+  case MachineOperand::MO_Register:
+    if (TargetRegisterInfo::isPhysicalRegister(MO.getReg()))
+      operand = "%" + LowercaseString (RI.get(MO.getReg()).AsmName);
+    else
+      operand = "%reg" + MO.getReg();
+    break;

Virtregs can never get to the asmprinter, please turn the "if" into an  
assert.




+  if (getTargetMachine().getRelocationModel() == Reloc::PIC_) {
+    SDValue GlobalBase = DAG.getNode(SPISD::GLOBAL_BASE_REG, dl,
+				     getPointerTy());
+    SDValue RelAddr = DAG.getNode(ISD::ADD, dl, MVT::i32, Lo, Hi);
+    SDValue AbsAddr = DAG.getNode(ISD::ADD, dl, MVT::i32,
+				  GlobalBase, RelAddr);
+    return DAG.getLoad(getPointerTy(), dl, DAG.getEntryNode(),
+		       AbsAddr, NULL, 0);
+  } else {
+    return DAG.getNode(ISD::ADD, dl, MVT::i32, Lo, Hi);
+  }

This code would be less nested if you wrote it as:

+  if (getTargetMachine().getRelocationModel() != Reloc::PIC_)
+    return DAG.getNode(ISD::ADD, dl, MVT::i32, Lo, Hi);

+  SDValue GlobalBase = DAG.getNode(SPISD::GLOBAL_BASE_REG, dl,
+		     getPointerTy());
+  SDValue RelAddr = DAG.getNode(ISD::ADD, dl, MVT::i32, Lo, Hi);
+  SDValue AbsAddr = DAG.getNode(ISD::ADD, dl, MVT::i32,
+		  GlobalBase, RelAddr);
+  return DAG.getLoad(getPointerTy(), dl, DAG.getEntryNode(),
+       AbsAddr, NULL, 0);





Some more important issues:

We need testcases for this :)


+    O << "\t.section\t\".text\"\n"

Please don't switch sections textually, use OutStreamer.SwitchSection.

<< "\t.align 4\n"

Likewise, use the streamer to emit the alignment.


More generally, I don't understand why LabelForBaseReg needs to be a  
map from strings to unsigned.  Can't you make it a map from MO.getReg 
() or something?  Can you give an example of the sort of code you're  
generating (I'm not familiar with sparc pic codegen).

Thanks,

-Chris