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

[Eric Christopher]

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)
>
>
> This seems like a strange idiom. Is it the norm? (hmm, looked myself - most
> code seems to do this as two lookups. hasAttr, then getAttr+getValueAsString
> - which isn't really ideal... oh, and I see this particular cliche of using
> "hasAttribute(Attribute::None)" is the norm in this particular file... OK
> then :))

Yeah. I'm not happy with the idiom myself. It's used all over the
place at the moment. One of those places that an Optional<T> would
make a little nicer for sure.

>
>
> (should Attribute be boolean testable for "presence"? then this could be
> written as:
>
>   bool softFloat = Options.UseSoftFloat;
>   if (Attribute SFAttr = FnAttr.getAttribute(AttributeSet::FunctionIndex,
> "use-soft-float"))
>     softFloat = SFAttr.getValueAsString() == "true";
>
> perhaps?)
>
>>
>> +                       ? (SFAttr.getValueAsString() == "true" ? true :
>> false)
>
>
> This is a little redundant. "SFAttr.getValueAsString() == "true" is already
> true/false, no need for the extra conditional operator, etc.
>

Thanks. Fixed in r218643.

>>
>> +                       : 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")];
>
>
> Hadn't gotten around to looking at the original patch that introduced the
> SubtargetMap - but I assume you've plans to replace this with something
> better at some point? (to avoid building new strings each time and
> canonicalize the features, etc (if there's any canonicalization required? I
> don't know))

Open to ideas on better. Canonicalization might be a bit more
difficult given strings, but it is possible.

The basic idea is that CPU should be a single string, FS will be a
longer string of things like "+walrus,+foo,+bar,+baz", then any
attribute that's on the function that would be involved in
initializing a subtarget. Right now it's just soft-float for Mips as
far as I've been able to determine, but a more thorough audit should
happen. Other than duplication between the existing FS/soft-float
options and added ones I can't come up with any canonicalization that
would work - though that is on the plate, it'll just take some work,
but shouldn't change generated code.

-eric

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