<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 29, 2014 at 4:42 PM, 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="">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>
><br>
><br>
> This seems like a strange idiom. Is it the norm? (hmm, looked myself - most<br>
> code seems to do this as two lookups. hasAttr, then getAttr+getValueAsString<br>
> - which isn't really ideal... oh, and I see this particular cliche of using<br>
> "hasAttribute(Attribute::None)" is the norm in this particular file... OK<br>
> then :))<br>
<br>
</span>Yeah. I'm not happy with the idiom myself. It's used all over the<br>
place at the moment. One of those places that an Optional<T> would<br>
make a little nicer for sure.<br>
<span class=""><br>
><br>
><br>
> (should Attribute be boolean testable for "presence"? then this could be<br>
> written as:<br>
><br>
> bool softFloat = Options.UseSoftFloat;<br>
> if (Attribute SFAttr = FnAttr.getAttribute(AttributeSet::FunctionIndex,<br>
> "use-soft-float"))<br>
> softFloat = SFAttr.getValueAsString() == "true";<br>
><br>
> perhaps?)<br>
><br>
>><br>
>> + ? (SFAttr.getValueAsString() == "true" ? true :<br>
>> false)<br>
><br>
><br>
> This is a little redundant. "SFAttr.getValueAsString() == "true" is already<br>
> true/false, no need for the extra conditional operator, etc.<br>
><br>
<br>
</span>Thanks. Fixed in r218643.<br>
<span class=""><br>
>><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>
><br>
><br>
> Hadn't gotten around to looking at the original patch that introduced the<br>
> SubtargetMap - but I assume you've plans to replace this with something<br>
> better at some point? (to avoid building new strings each time and<br>
> canonicalize the features, etc (if there's any canonicalization required? I<br>
> don't know))<br>
<br>
</span>Open to ideas on better. Canonicalization might be a bit more<br>
difficult given strings, but it is possible.<br></blockquote><div><br></div><div>Fair enough - though I assume /something/ parses these at some point, so maybe we should be keeping that structure around rather than strings - but I don't know.<br><br>If it stays as strings, yeah, canonicalization might not be practical - but rather than concatenating, it might be reasonable to just have a struct with StringRefs in it* & use that as a key instead.<br><br>* As (I think Tom) someone else pointed out in another thread, I guess the other features (like hard/soft float) etc are target specific, so it might not be possible to enumerate a structure and could instead end up as a vector of StringRefs. No idea if that's a step up/forward, etc...<br><br>It's a pretty isolated/easily refactored design decision anyway, so I'm not too fussed about it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The basic idea is that CPU should be a single string, FS will be a<br>
longer string of things like "+walrus,+foo,+bar,+baz", then any<br>
attribute that's on the function that would be involved in<br>
initializing a subtarget. Right now it's just soft-float for Mips as<br>
far as I've been able to determine, but a more thorough audit should<br>
happen. Other than duplication between the existing FS/soft-float<br>
options and added ones I can't come up with any canonicalization that<br>
would work - though that is on the plate, it'll just take some work,<br>
but shouldn't change generated code.<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>><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>