[llvm-commits] PATCH: SPARC codegen fixes

[Venkatraman Govindaraju]

Is this patch okay to commit?

Thanks,
Venkatraman

On Fri, Aug 28, 2009 at 10:50 PM, Venkatraman
Govindaraju<venkatra at cs.wisc.edu> wrote:
> 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
>>
>>
>