[llvm-commits] [llvm] r80728 - /llvm/trunk/lib/Target/X86/X86CodeEmitter.cpp

[Bruno Cardoso Lopes]

On Tue, Sep 1, 2009 at 10:51 PM, Daniel Dunbar<daniel at zuster.org> wrote:
> On Tue, Sep 1, 2009 at 3:56 PM, Bruno Cardoso
> Lopes<bruno.cardoso at gmail.com> wrote:
>> Hi Daniel,
>>
>> On Tue, Sep 1, 2009 at 7:07 PM, Daniel Dunbar<daniel at zuster.org> wrote:
>>> Author: ddunbar
>>> Date: Tue Sep  1 17:07:00 2009
>>> New Revision: 80728
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=80728&view=rev
>>> Log:
>>> Fix what I believe is a copy-n-pasto introduced in r78129.
>>>  - Bruno, please check!!
>>>
>>> Modified:
>>>    llvm/trunk/lib/Target/X86/X86CodeEmitter.cpp
>>>
>>> Modified: llvm/trunk/lib/Target/X86/X86CodeEmitter.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86CodeEmitter.cpp?rev=80728&r1=80727&r2=80728&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/X86/X86CodeEmitter.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/X86CodeEmitter.cpp Tue Sep  1 17:07:00 2009
>>> @@ -340,18 +340,18 @@
>>>   } else if (RelocOp->isSymbol()) {
>>>     unsigned rt = Is64BitMode ?
>>>       (IsPCRel ? X86::reloc_pcrel_word : X86::reloc_absolute_word_sext)
>>> -      : (IsPCRel ? X86::reloc_picrel_word : X86::reloc_absolute_word);
>>> +      : (IsPIC ? X86::reloc_picrel_word : X86::reloc_absolute_word);
>>
>> It's not a copy-n-pasto. The old behavior in r78129 was:
>>
>> unsigned rt = Is64BitMode ? X86::reloc_pcrel_word : X86::reloc_picrel_word;
>>
>> That is, to use reloc_picrel_word for "!Is64BitMode" even if we're not
>> in PIC mode.
>
> Ok. Disregarding that, is it correct? :)

Ignoring that I believe it's ok.

> Since the various encodings of a MachineOperand all amount to similar
> concepts, it seems unlikely that they should emit different relocation
> types. Also note that very similar code in other places, for example
> for the MRM?m: encodings, uses this logic:
> --
>    unsigned rt = Is64BitMode ? X86::reloc_pcrel_word
>      : (IsPIC ? X86::reloc_picrel_word : X86::reloc_absolute_word);
>    if (Opcode == X86::MOV64mi32)
>      rt = X86::reloc_absolute_word_sext;  // FIXME: add X86II flag?
>    if (MO.isGlobal()) {
>      bool NeedStub = isa<Function>(MO.getGlobal());
>      bool Indirect = gvNeedsNonLazyPtr(MO, TM);
>      emitGlobalAddress(MO.getGlobal(), rt, MO.getOffset(), 0,
>                        NeedStub, Indirect);
> --
> which does check IsPIC. Similarly MRM?r encodings have a near copy of
> this code that uses IsPIC.

Makes sense

> One way or the other, I'm pretty sure that all copies of this code
> should use the same logic, I'm just not sure which is right.

I agree but I'm not sure too, I don't know if there was any specific
reason for it
to be this way when it was first written. This is why I left the old behaviour
when I applied r78129.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc