<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 6, 2014 at 10:26 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Oct 6, 2014 at 9:37 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Sep 29, 2014 at 2:57 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Author: echristo<br>
>> Date: Mon Sep 29 16:57:54 2014<br>
>> New Revision: 218632<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=218632&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=218632&view=rev</a><br>
>> Log:<br>
>> Add soft-float to the key for the subtarget lookup in the TargetMachine<br>
>> map, this makes sure that we can compile the same code for two different<br>
>> ABIs (hard and soft float) in the same module.<br>
><br>
><br>
> Not sure I asked/gave the credit here - does this mean hard/soft float<br>
> handling has transitioned to your new varying-subtarget infrastructure? (&<br>
> the test demonstrates that it's not just a drop-in replacement for existing<br>
> functionality, but addresses bugs in the previous implementation?)<br>
><br>
<br>
</span>Correct.<br>
<span class=""><br>
> Oh, I see, it was 218492 - awesome. So that actually enabled per-function<br>
> cpu feature attributes & such on MIPS? Very cool. (sounds like that change<br>
> could've had at least one test that demonstrated the per-function attributes<br>
> varying - your test change, while necessary for the tests to pass after the<br>
> change (& thus, somewhat demonstrating the change in behavior) would now<br>
> equally pass before your change, if I understand correctly - so not really<br>
> testing to ensure we don't regress this behavior)<br>
><br>
<br>
</span>Correct. Well, it had plenty because of all the mips16 tests, I just<br>
had a bug with the subtarget with soft float bit :)<br>
<br>
Basically the bug was that there was a piece of global state that was<br>
being used in subtarget initialization that I wasn't paying attention<br>
to in per-function subtarget initialization. Also, it wouldn't have<br>
been really noticed until someone using the mips backend decided to<br>
LTO some functions together that were soft-float/hard-float and<br>
mips16. But it was there and should be fixed.<br></blockquote><div><br></div><div>Right - and your extra (reordered) test case does expose the bug. I had something confused in thinking the test cases would pass both before and after your change (maybe I was getting some of the changes confused with your other commit *goes to look again*)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> (do we get to delete any code that was handling this in other ways?)<br>
<br>
</span>Sadly, not yet. That's what that obnoxious todo means. But yes, we<br>
will get to eventually I think. UseSoftFloat should probably disappear<br>
from the Options struct and just be on the function. The only reason I<br>
haven't done this yet is that it's not a bad place to put a global<br>
default for the target either.<br>
<br>
Make sense?<br></blockquote><div><br></div><div>Vaguely (only due to my only vague understanding of this code in general - only going off what we've discussed previously for the most part). Thanks for the info.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
><br>
>><br>
>> Update one testcase accordingly (and fix some confusing naming) and<br>
>> add a new testcase as well with the ordering swapped which would<br>
>> highlight the problem.<br>
>><br>
>> Added:<br>
>> llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll<br>
>> - copied, changed from r218631,<br>
>> llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll<br>
>> Modified:<br>
>> llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp<br>
>> llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll<br>
>><br>
>> Modified: llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp?rev=218632&r1=218631&r2=218632&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp?rev=218632&r1=218631&r2=218632&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp (original)<br>
>> +++ llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp Mon Sep 29 16:57:54<br>
>> 2014<br>
>> @@ -105,12 +105,24 @@ MipsTargetMachine::getSubtargetImpl(cons<br>
>> !FnAttrs.getAttribute(AttributeSet::FunctionIndex, "nomips16")<br>
>> .hasAttribute(Attribute::None);<br>
>><br>
>> + // FIXME: This is related to the code below to reset the target<br>
>> options,<br>
>> + // we need to know whether or not the soft float flag is set on the<br>
>> + // function before we can generate a subtarget. We also need to use<br>
>> + // it as a key for the subtarget since that can be the only difference<br>
>> + // between two functions.<br>
>> + Attribute SFAttr =<br>
>> + FnAttrs.getAttribute(AttributeSet::FunctionIndex,<br>
>> "use-soft-float");<br>
>> + bool softFloat = !SFAttr.hasAttribute(Attribute::None)<br>
>> + ? (SFAttr.getValueAsString() == "true" ? true :<br>
>> false)<br>
>> + : Options.UseSoftFloat;<br>
>> +<br>
>> if (hasMips16Attr)<br>
>> FS += FS.empty() ? "+mips16" : ",+mips16";<br>
>> else if (hasNoMips16Attr)<br>
>> FS += FS.empty() ? "-mips16" : ",-mips16";<br>
>><br>
>> - auto &I = SubtargetMap[CPU + FS];<br>
>> + auto &I = SubtargetMap[CPU + FS + (softFloat ? "use-soft-float=true"<br>
>> + :<br>
>> "use-soft-float=false")];<br>
>> if (!I) {<br>
>> // This needs to be done before we create a new subtarget since any<br>
>> // creation will depend on the TM and the code generation flags on<br>
>> the<br>
>><br>
>> Copied: llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll (from r218631,<br>
>> llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll)<br>
>> URL:<br>
>> <a href="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" target="_blank">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</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll (original)<br>
>> +++ llvm/trunk/test/CodeGen/Mips/mips16-hf-attr-2.ll Mon Sep 29 16:57:54<br>
>> 2014<br>
>> @@ -3,16 +3,16 @@<br>
>> ; RUN: llc -mtriple=mipsel-linux-gnu -march=mipsel \<br>
>> ; RUN: -mcpu=mips16 -relocation-model=pic < %s | FileCheck %s<br>
>><br>
>> -define void @bar_sf() #0 {<br>
>> +define void @bar_sf() #1 {<br>
>> ; CHECK: bar_sf:<br>
>> entry:<br>
>> %call1 = call float @foo(float 1.000000e+00)<br>
>> -; CHECK: lw $2, %call16(foo)($3)<br>
>> -; CHECK: lw $5, %got(__mips16_call_stub_sf_1)($3)<br>
>> +; CHECK: lw $3, %call16(foo)($2)<br>
>> +; CHECK-NOT: lw $5, %got(__mips16_call_stub_sf_1)($3)<br>
>> ret void<br>
>> }<br>
>><br>
>> -define void @bar_hf() #1 {<br>
>> +define void @bar_hf() #0 {<br>
>> ; CHECK: bar_hf:<br>
>> entry:<br>
>> %call1 = call float @foo(float 1.000000e+00)<br>
>><br>
>> Modified: llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll?rev=218632&r1=218631&r2=218632&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll?rev=218632&r1=218631&r2=218632&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll (original)<br>
>> +++ llvm/trunk/test/CodeGen/Mips/mips16-hf-attr.ll Mon Sep 29 16:57:54<br>
>> 2014<br>
>> @@ -3,8 +3,8 @@<br>
>> ; RUN: llc -mtriple=mipsel-linux-gnu -march=mipsel \<br>
>> ; RUN: -mcpu=mips16 -relocation-model=pic < %s | FileCheck %s<br>
>><br>
>> -define void @bar_sf() #0 {<br>
>> -; CHECK: bar_sf:<br>
>> +define void @bar_hf() #0 {<br>
>> +; CHECK: bar_hf:<br>
>> entry:<br>
>> %call1 = call float @foo(float 1.000000e+00)<br>
>> ; CHECK: lw $2, %call16(foo)($3)<br>
>> @@ -12,12 +12,12 @@ entry:<br>
>> ret void<br>
>> }<br>
>><br>
>> -define void @bar_hf() #1 {<br>
>> -; CHECK: bar_hf:<br>
>> +define void @bar_sf() #1 {<br>
>> +; CHECK: bar_sf:<br>
>> entry:<br>
>> %call1 = call float @foo(float 1.000000e+00)<br>
>> -; CHECK: lw $2, %call16(foo)($3)<br>
>> -; CHECK: lw $5, %got(__mips16_call_stub_sf_1)($3)<br>
>> +; CHECK: lw $3, %call16(foo)($2)<br>
>> +; CHECK-NOT: lw $5, %got(__mips16_call_stub_sf_1)($3)<br>
>> ret void<br>
>> }<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>