[llvm-commits] PATCH: SPARC codegen fixes

[Venkatraman Govindaraju]

On Fri, Aug 28, 2009 at 12:00 AM, Chris Lattner<clattner at apple.com> wrote:
>
>
> Whoa, nice!  Some minor stylistic comments:
>
> +let Defs = [ICC] in {
> +    defm SUBCC  : F3_12  <"subcc", 0b010100, SPcmpicc>;
>
> Please indent by 2.
>

Done.

> +  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
>

Deleted

> +  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))
>

See below. I don't need LabelForBaseReg anymore.

>
> +  // Print out PIC Base computation helpers
>
> Please properly punctuate the sentence (end with .)
>
>
> Please don't use tabs in the code.
>

Removed tabs that I found.

>
> +  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.
>

Done.

>
>
>
> +  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);
>

Done.

>
>
>
>
> Some more important issues:
>
> We need testcases for this :)
>

I added two testcases. First is to test whether function linkage is
correctly set. Second is to test the PIC code generates code to get
_GLOBAL_OFFSET_TABLE_.
Please let me know how to make them better or need more testcases.

>
> +    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).
>

I modified the code so that I don't need LabelForBaseReg any more. I
also don't need to change the sections manually.
I am generating the following code for PIC. This example assumes that
register %l7 is allocated to be the base register

foo:
     //
     Function Prolog
     //
.LLGETPCH0:
      // Call sets PC to %o7
      call .LLGETPC0
      // This sequence computes _GLOBAL_OFFSET_TABLE_ offset from call
instruction's PC
      sethi %hi(_GLOBAL_OFFSET_TABLE_+(.-.LLGETPC0), %l7
      or %l7, %lo(_GLOBAL_OFFSET_TABLE_+(.-.LLGETPC0), %l7
.LLGETPC0:
       // Finally add the PC to get absolute address of
_GLOBAL_OFFSET_TABLE_ in %l7
       add %l7, %o7, %l7

       // Loading a global value to scratch register %g1
       // Load the glob ptr's offset
       sethi %hi(global), %g1
       add %g1, %lo(global), %g1
       // Load the glob ptr from _GLOBAL_OFFSET_TABLE_
       ld [%g1+%l7], %g1
      // Finally load the value itself.
       ld [%g1], %g1

> Thanks,
>
> -Chris
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sparc-codegen-pic.patch
Type: application/octet-stream
Size: 20342 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090828/d015b74e/attachment.obj>