[llvm-commits] [Mips] Direct object big endian review and submittal request

[Carter, Jack]

Here is my re-submittal with annotation. Keep in mind that my work is on the direct object emission of Mips code and thus an assembler .s file is never created.

Thanks for the feedback. It has made me think harder.

**********************
test/MC/Mips/elf-64bitEL.ll
Check for ELF header marking this 64 bit
Check that it is little endian
Check that the text section header is present and the type,flags and alignment is correct. The sh_entsize is NOT correct and I have marked that on my todo list.
Check that the first words of the text section, GP prolog are correct.
Check that one of the sections/tables that will be affected by 64 bit is correct. I picked the symbol table since each of its' entries will be twice as big as the 32 bit flavor.

**********************
test/MC/Mips/elf-bigendian.ll
Check that the ELF header marks this as big endian
Check that the text section header is present, readable and the type,flags and alignment is correct. The sh_entsize is NOT correct and I have marked that on my todo list.
Check that the first words of the text section, GP prolog are correct. If the endianess is done wrong this test will fail.

**********************
test/MC/Mips/elf-tls.ll
Redid this test to allow for padded '0's. I don't know why I get them now with elf-dump when I didn't before, but life is too short and besides it help me with test creation ;-)

**********************
lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp
I changed the TargetRegistry to invoke targets for endianess and 32/64.

The most controversial part about this is that I decided to not split up the variants into subclasses and went instead with different "create" routines that changed the single class parameters. It just was less confusing to me.

This hopefully will become moot as I want to instead pass a reference to the fully fleshed out SubtargetInfo class object so target information is available to me no matter which of the myriad Mips target variants I have to process and note.

**********************
lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp

In addition to the class vs. create issues described above this change also has  some issues of note. In ApplyFixup the instruction (or data) steam is already endianized. Way to early for my taste, but such is it currently. I de-endianize the target object, usually a 32 bit instruction, apply the fixup, and then re-endianize it.

This brings up 2 questions:
1: Why do I need to worry about the endianization here?
    I shouldn't.
    It should be handled later on right before output.
2: Why can't I use the "bits" field from:
    NumBytes = ((getFixupKindInfo(Kind).TargetSize-1)/8)+1;

    I get pointed at the whole object which has already been endianized.

    The fixup value is in platform endian which in our current case mean
    little endian.

    In order to do the fixup the value needs to be found in
    the object and converted to the target endian.

    The full object is endianized  putting the target fixup
    where we expect it and the fixup value is added
    (not inserted) to the value already in that location.

    Since we had to de-endianize the whole object, we now need to re-endianize it.

    If I suppose since I know the object size from the relocation type
    I could grab the exact number of bytes needed and de-endianize
    them, but then I would need to break the routine up into 16,32 and
    64 bit variants, probably with templates. I will have to put this into
    my todo list for later. What I have now seems to work on our testing.

**********************
lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp

Put in 64 bit variant into MipsELFObjectWriter class parameters and passed to createELFObjectWriter() so the ELF header will get the setting.

**********************
lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.h

Declarations for .cpp changes above and some endian macros.

Note: these macros should go away for the appropriate llvm variants. I didn't find ones that matched exactly what I wanted. I have a FIXME in the code for this.

**********************
lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp

Put in big endian support.

Note: I don't think it should be done this low, but it is how llvm is set up at this time.

I also added a FIXME for a non-related switch case that I suspect needs to generate an assertion, but have not tested my hunch yet.

Jack

________________________________
From: Carter, Jack
Sent: Wednesday, January 04, 2012 8:33 PM
To: llvm-commits at cs.uiuc.edu
Cc: bruno.cardoso at gmail.com; Hatanaka, Akira; Kotler, Reed
Subject: [Mips] Direct object big endian review and submittal request

The bigest changes are to endianize the opcode and then the fixups. I am less than thrilled
that we have to do this at this level of the compiler and not at a higher level, but that
will be a battle for another day.

I am also concerned that there seems to be an assumption that the host machine will be
little endian. Instead of checking if the target is big or little endian I would have
thought we would be asking if it was a different endian or not.

Contributers: Jack Carter

#
#    modified:   MCTargetDesc/MipsAsmBackend.cpp
#    modified:   MCTargetDesc/MipsMCCodeEmitter.cpp
#    modified:   MCTargetDesc/MipsMCTargetDesc.cpp
#    modified:   MCTargetDesc/MipsMCTargetDesc.h
#    new        :   test/MC/Mips/elf-bigendian.ll

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120110/56cea9f1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: big_endian.patch
Type: text/x-patch
Size: 23344 bytes
Desc: big_endian.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120110/56cea9f1/attachment.bin>