[llvm] r218632 - Add soft-float to the key for the subtarget lookup in the TargetMachine

[Eric Christopher]

On Mon, Oct 6, 2014 at 9:37 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Sep 29, 2014 at 2:57 PM, Eric Christopher <echristo at gmail.com>
> wrote:
>>
>> Author: echristo
>> Date: Mon Sep 29 16:57:54 2014
>> New Revision: 218632
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=218632&view=rev
>> Log:
>> Add soft-float to the key for the subtarget lookup in the TargetMachine
>> map, this makes sure that we can compile the same code for two different
>> ABIs (hard and soft float) in the same module.
>
>
> Not sure I asked/gave the credit here - does this mean hard/soft float
> handling has transitioned to your new varying-subtarget infrastructure? (&
> the test demonstrates that it's not just a drop-in replacement for existing
> functionality, but addresses bugs in the previous implementation?)
>

Correct.

> Oh, I see, it was 218492 - awesome. So that actually enabled per-function
> cpu feature attributes & such on MIPS? Very cool. (sounds like that change
> could've had at least one test that demonstrated the per-function attributes
> varying - your test change, while necessary for the tests to pass after the
> change (& thus, somewhat demonstrating the change in behavior) would now
> equally pass before your change, if I understand correctly - so not really
> testing to ensure we don't regress this behavior)
>

Correct. Well, it had plenty because of all the mips16 tests, I just
had a bug with the subtarget with soft float bit :)

Basically the bug was that there was a piece of global state that was
being used in subtarget initialization that I wasn't paying attention
to in per-function subtarget initialization. Also, it wouldn't have
been really noticed until someone using the mips backend decided to
LTO some functions together that were soft-float/hard-float and
mips16. But it was there and should be fixed.

> (do we get to delete any code that was handling this in other ways?)

Sadly, not yet. That's what that obnoxious todo means. But yes, we
will get to eventually I think. UseSoftFloat should probably disappear
from the Options struct and just be on the function. The only reason I
haven't done this yet is that it's not a bad place to put a global
default for the target either.

Make sense?

-eric


>
>>
>> Update one testcase accordingly (and fix some confusing naming) and
>> add a new testcase as well with the ordering swapped which would
>> highlight the problem.
>>
>> Added:
>>     llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll
>>       - copied, changed from r218631,
>> llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll
>> Modified:
>>     llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp
>>     llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp?rev=218632&r1=218631&r2=218632&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp Mon Sep 29 16:57:54
>> 2014
>> @@ -105,12 +105,24 @@ MipsTargetMachine::getSubtargetImpl(cons
>>        !FnAttrs.getAttribute(AttributeSet::FunctionIndex, "nomips16")
>>             .hasAttribute(Attribute::None);
>>
>> +  // FIXME: This is related to the code below to reset the target
>> options,
>> +  // we need to know whether or not the soft float flag is set on the
>> +  // function before we can generate a subtarget. We also need to use
>> +  // it as a key for the subtarget since that can be the only difference
>> +  // between two functions.
>> +  Attribute SFAttr =
>> +      FnAttrs.getAttribute(AttributeSet::FunctionIndex,
>> "use-soft-float");
>> +  bool softFloat = !SFAttr.hasAttribute(Attribute::None)
>> +                       ? (SFAttr.getValueAsString() == "true" ? true :
>> false)
>> +                       : Options.UseSoftFloat;
>> +
>>    if (hasMips16Attr)
>>      FS += FS.empty() ? "+mips16" : ",+mips16";
>>    else if (hasNoMips16Attr)
>>      FS += FS.empty() ? "-mips16" : ",-mips16";
>>
>> -  auto &I = SubtargetMap[CPU + FS];
>> +  auto &I = SubtargetMap[CPU + FS + (softFloat ? "use-soft-float=true"
>> +                                               :
>> "use-soft-float=false")];
>>    if (!I) {
>>      // This needs to be done before we create a new subtarget since any
>>      // creation will depend on the TM and the code generation flags on
>> the
>>
>> Copied: llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll (from r218631,
>> llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll)
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll?p2=llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll&p1=llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll&r1=218631&r2=218632&rev=218632&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll (original)
>> +++ llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll Mon Sep 29 16:57:54
>> 2014
>> @@ -3,16 +3,16 @@
>>  ; RUN: llc -mtriple=mipsel-linux-gnu -march=mipsel \
>>  ; RUN:     -mcpu=mips16 -relocation-model=pic < %s | FileCheck %s
>>
>> -define void @bar_sf() #0 {
>> +define void @bar_sf() #1 {
>>  ; CHECK: bar_sf:
>>  entry:
>>    %call1 = call float @foo(float 1.000000e+00)
>> -; CHECK: lw $2, %call16(foo)($3)
>> -; CHECK: lw $5, %got(__mips16_call_stub_sf_1)($3)
>> +; CHECK: lw $3, %call16(foo)($2)
>> +; CHECK-NOT: lw $5, %got(__mips16_call_stub_sf_1)($3)
>>    ret void
>>  }
>>
>> -define void @bar_hf() #1 {
>> +define void @bar_hf() #0 {
>>  ; CHECK: bar_hf:
>>  entry:
>>    %call1 = call float @foo(float 1.000000e+00)
>>
>> Modified: llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll?rev=218632&r1=218631&r2=218632&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll (original)
>> +++ llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll Mon Sep 29 16:57:54
>> 2014
>> @@ -3,8 +3,8 @@
>>  ; RUN: llc -mtriple=mipsel-linux-gnu -march=mipsel \
>>  ; RUN:     -mcpu=mips16 -relocation-model=pic < %s | FileCheck %s
>>
>> -define void @bar_sf() #0 {
>> -; CHECK: bar_sf:
>> +define void @bar_hf() #0 {
>> +; CHECK: bar_hf:
>>  entry:
>>    %call1 = call float @foo(float 1.000000e+00)
>>  ; CHECK: lw $2, %call16(foo)($3)
>> @@ -12,12 +12,12 @@ entry:
>>    ret void
>>  }
>>
>> -define void @bar_hf() #1 {
>> -; CHECK: bar_hf:
>> +define void @bar_sf() #1 {
>> +; CHECK: bar_sf:
>>  entry:
>>    %call1 = call float @foo(float 1.000000e+00)
>> -; CHECK: lw $2, %call16(foo)($3)
>> -; CHECK: lw $5, %got(__mips16_call_stub_sf_1)($3)
>> +; CHECK: lw $3, %call16(foo)($2)
>> +; CHECK-NOT: lw $5, %got(__mips16_call_stub_sf_1)($3)
>>    ret void
>>  }
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>