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

Tue Sep 30 09:20:52 PDT 2014

On Mon, Sep 29, 2014 at 4:42 PM, Eric Christopher <echristo at gmail.com>
wrote:

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

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.

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.

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

It's a pretty isolated/easily refactored design decision anyway, so I'm not
too fussed about it.

>
> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140930/44d1a111/attachment.html>