[llvm] r210414 - Do materialize for floating point

Reed Kotler Reed.Kotler at imgtec.com
Sun Jun 8 03:39:19 PDT 2014


Okay.

I will redo the patch later today.

It's nice that you show an interest in what is going on here.
I don't want to discourage that.

I admit to being sloppy with the checkin comments in Phabricator which unfortunately ended up making
them look funny in the LLVM putback. Daniel has been EXTREMELY busy lately
with some very demanding project schedules for his own work and my patches have backed up awaiting review
and this weekend I wanted to push the ones that were recently approved and also do a lot of other
new work on the Mips fast-isel. I had a few hours today for doing that but I also have to rebuild and retest
things before doing that so it ate up these few hours.

The problems are not related to LLVM but to getting a good work flow with Phabricator in these situations
where patches that often overlap, start to back up. Daniel has suggested a good work flow for this and
hopefully it will make things better in the future as I start to do things that way.

I have set some aggressive personal goals for percent completion for this Mips fast-isel pass so I am trying to wrap up a lot of lose ends within the next few weeks. So perhaps I'm a bit rushed but these patches are fine and they have good make check test cases and I have good executable test cases
for them too.


Reed


________________________________________
From: Alp Toker [alp at nuanti.com]
Sent: Sunday, June 08, 2014 3:23 AM
To: Reed Kotler; Daniel Sanders; llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r210414 - Do materialize for floating point

On 08/06/2014 13:12, Reed Kotler wrote:
> This is a very trivial change to "Do materialize for floating point".

Hi Reed,

That's great. LGTM with that description!

Please use the code owner system responsibly in future to keep it
viable. It shouldn't be used to cover up situations like:

   "I did not understand the comment about using dyn_cast instead of
isa. I will commit as is and make the update after."

Alp.




>
> If you understand what fast-isel does, that is all you need to say.
> Each port has a materialize function for floating point literals.
> That is the summary of the check in. It implements materialize for
> floating point.
>
> I'm not used to using Phabricator yet but checked in a whole code
> generator for Mips16
> on my own and passed all of test suite without any problems.
>
> I have a lot of changes backed up right now and don't have the perfect
> work flow
> with Daniel yet to deal with that and all these deltas and keep
> phabricator straight
> at check in time. I'm doing the best I can there but it's not a
> requirement and most people
> are not even using phabricator at this time.
>
> Phabricator is being included just to make it easier for people to see
> the checking
> without having to just look at the diffs.
>
> The commit is fine and it has very good test case.
>
> Reed
>
> On 06/08/2014 02:40 AM, Alp Toker wrote:
>>
>> On 08/06/2014 11:42, Reed Kotler wrote:
>>> The summary area in the putback comment was messed up but the patch
>>> is fine.
>>> I'm not very good with phabricator yet.
>>
>> Reed, I understand that your commit log was messed up but that's not
>> the actual problem.
>>
>> I've reverted the change in r210424 with a full explanation and will
>> follow up shortly to the list.
>>
>> Alp.
>>
>>
>>>
>>> I don't understand your concerns.
>>>
>>> You should look at the other fast-isel implementations, i.e. ppc,
>>> arm and x86.
>>>
>>> This one is almost identical.
>>>
>>> All the implementations are all essentially the same.
>>>
>>> You are reading too much into the comment "creates lot of core
>>> infrastructure".
>>> It's not "core infrastructure" for llvm in general.
>>>
>>> This comment actually applied to an earlier commit. I have a lot of
>>> patches backed up for
>>> review and phabricator was seeing earlier comments and I should have
>>> deleted them
>>> but forgot to.
>>>
>>> The only thing this patch does is to add the additional
>>> functionality for materializing
>>> floats.
>>>
>>> Reed
>>>
>>> On 06/08/2014 12:11 AM, Alp Toker wrote:
>>>> Hi Daniel,
>>>>
>>>> I'm concerned about the quality of your review...
>>>>
>>>>
>>>> On 08/06/2014 06:30, Reed Kotler wrote:
>>>>> Author: rkotler
>>>>> Date: Sat Jun  7 22:30:32 2014
>>>>> New Revision: 210414
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=210414&view=rev
>>>>> Log:
>>>>> Do materialize for floating point
>>>>>
>>>>> Summary:
>>>>> start to do simple constants
>>>>>
>>>>> finish simplestore
>>>>>
>>>>> add test case
>>>>>
>>>>> format
>>>>>
>>>>> Merge branch 'master' into 1756_8
>>>>>
>>>>> Add basic functionality for assignment of ints. This creates a lot
>>>>> of core infrastructure in which to add, with little effort, quite
>>>>> a bit more to mips fast-isel
>>>>
>>>> A patch that "creates lot of core infrastructure" should have been
>>>> reviewed on-list.
>>>>
>>>>>
>>>>> Merge branch 'master' into 1756_8
>>>>>
>>>>> Add basic functionality for assignment of ints. This creates a lot
>>>>> of core infrastructure in which to add, with little effort, quite
>>>>> a bit more to mips fast-isel
>>>>>
>>>>> in progress
>>>>>
>>>>> finish integer materialize
>>>>>
>>>>> test cases
>>>>>
>>>>> test cases
>>>>>
>>>>> in progress
>>>>>
>>>>> Finish up fast-isel materialize for ints.
>>>>>
>>>>> Finish materialize for ints
>>>>>
>>>>> test cases
>>>>>
>>>>> simplestorei.ll
>>>>>
>>>>> Merge branch 'master' into 1756_8
>>>>>
>>>>> fix fp constants for fast-isel
>>>>>
>>>>> Merge branch '1758_1' of dmz-portal.mips.com:llvm into 1758_1
>>>>>
>>>>> in progress
>>>>>
>>>>> lastest for fp materialization
>>>>>
>>>>> clean up
>>>>>
>>>>> Merge branch 'master' into 1758_1
>>>>>
>>>>> formatting
>>>>>
>>>>> add test case
>>>>>
>>>>> finish test case
>>>>>
>>>>> Merge branch 'master' into 1758_2
>>>>>
>>>>> Test Plan:
>>>>> simplestore.ll
>>>>>
>>>>> simplestore.ll
>>>>>
>>>>> Reviewers: dsanders
>>>>>
>>>>> Reviewed By: dsanders
>>>>
>>>> ?
>>>>
>>>>>
>>>>> Differential Revision: http://reviews.llvm.org/D3659
>>>>
>>>> If I'm reading correctly, the quality issues were identified and
>>>> just ignored to get a final LGTM and commit.
>>>>
>>>> What's going on here?
>>>>
>>>> Alp.
>>>>
>>>>
>>>>>
>>>>> Added:
>>>>> llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll
>>>>> Modified:
>>>>>      llvm/trunk/lib/Target/Mips/MipsFastISel.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/Target/Mips/MipsFastISel.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsFastISel.cpp?rev=210414&r1=210413&r2=210414&view=diff
>>>>> ==============================================================================
>>>>>
>>>>> --- llvm/trunk/lib/Target/Mips/MipsFastISel.cpp (original)
>>>>> +++ llvm/trunk/lib/Target/Mips/MipsFastISel.cpp Sat Jun  7
>>>>> 22:30:32 2014
>>>>> @@ -167,9 +167,14 @@ bool MipsFastISel::EmitStore(MVT VT, uns
>>>>>     //
>>>>>     // more cases will be handled here in following patches.
>>>>>     //
>>>>> -  if (VT != MVT::i32)
>>>>> +  if (VT == MVT::i32)
>>>>> +    EmitInstStore(Mips::SW, SrcReg, Addr.Base.Reg, Addr.Offset);
>>>>> +  else if (VT == MVT::f32)
>>>>> +    EmitInstStore(Mips::SWC1, SrcReg, Addr.Base.Reg, Addr.Offset);
>>>>> +  else if (VT == MVT::f64)
>>>>> +    EmitInstStore(Mips::SDC1, SrcReg, Addr.Base.Reg, Addr.Offset);
>>>>> +  else
>>>>>       return false;
>>>>> -  EmitInstStore(Mips::SW, SrcReg, Addr.Base.Reg, Addr.Offset);
>>>>>     return true;
>>>>>   }
>>>>>   @@ -229,6 +234,22 @@ bool MipsFastISel::TargetSelectInstructi
>>>>>   }
>>>>>     unsigned MipsFastISel::MaterializeFP(const ConstantFP *CFP,
>>>>> MVT VT) {
>>>>> +  int64_t Imm = CFP->getValueAPF().bitcastToAPInt().getZExtValue();
>>>>> +  if (VT == MVT::f32) {
>>>>> +    const TargetRegisterClass *RC = &Mips::FGR32RegClass;
>>>>> +    unsigned DestReg = createResultReg(RC);
>>>>> +    unsigned TempReg = Materialize32BitInt(Imm,
>>>>> &Mips::GPR32RegClass);
>>>>> +    EmitInst(Mips::MTC1, DestReg).addReg(TempReg);
>>>>> +    return DestReg;
>>>>> +  } else if (VT == MVT::f64) {
>>>>> +    const TargetRegisterClass *RC = &Mips::AFGR64RegClass;
>>>>> +    unsigned DestReg = createResultReg(RC);
>>>>> +    unsigned TempReg1 = Materialize32BitInt(Imm >> 32,
>>>>> &Mips::GPR32RegClass);
>>>>> +    unsigned TempReg2 =
>>>>> +        Materialize32BitInt(Imm & 0xFFFFFFFF, &Mips::GPR32RegClass);
>>>>> +    EmitInst(Mips::BuildPairF64,
>>>>> DestReg).addReg(TempReg2).addReg(TempReg1);
>>>>> +    return DestReg;
>>>>> +  }
>>>>>     return 0;
>>>>>   }
>>>>>
>>>>> Added: llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll?rev=210414&view=auto
>>>>> ==============================================================================
>>>>>
>>>>> --- llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll (added)
>>>>> +++ llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll Sat
>>>>> Jun  7 22:30:32 2014
>>>>> @@ -0,0 +1,39 @@
>>>>> +; RUN: llc -march=mipsel -relocation-model=pic -O0
>>>>> -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
>>>>> +; RUN:     < %s | FileCheck %s
>>>>> +
>>>>> + at f = common global float 0.000000e+00, align 4
>>>>> + at de = common global double 0.000000e+00, align 8
>>>>> +
>>>>> +; Function Attrs: nounwind
>>>>> +define void @f1() #0 {
>>>>> +entry:
>>>>> +  store float 0x3FFA76C8C0000000, float* @f, align 4
>>>>> +  ret void
>>>>> +; CHECK:    .ent    f1
>>>>> +; CHECK:    lui    $[[REG1:[0-9]+]], 16339
>>>>> +; CHECK:    ori    $[[REG2:[0-9]+]], $[[REG1]], 46662
>>>>> +; CHECK:    mtc1    $[[REG2]], $f[[REG3:[0-9]+]]
>>>>> +; CHECK:    lw    $[[REG4:[0-9]+]], %got(f)(${{[0-9]+}})
>>>>> +; CHECK:    swc1    $f[[REG3]], 0($[[REG4]])
>>>>> +; CHECK:     .end    f1
>>>>> +
>>>>> +}
>>>>> +
>>>>> +; Function Attrs: nounwind
>>>>> +define void @d1() #0 {
>>>>> +entry:
>>>>> +  store double 1.234567e+00, double* @de, align 8
>>>>> +; CHECK:    .ent    d1
>>>>> +; CHECK:    lui    $[[REG1a:[0-9]+]], 16371
>>>>> +; CHECK:    ori    $[[REG2a:[0-9]+]], $[[REG1a]], 49353
>>>>> +; CHECK:    lui    $[[REG1b:[0-9]+]], 21403
>>>>> +; CHECK:    ori    $[[REG2b:[0-9]+]], $[[REG1b]], 34951
>>>>> +; CHECK:    mtc1    $[[REG2b]], $f[[REG3b:[0-9]+]]
>>>>> +; CHECK:    mtc1    $[[REG2a]], $f[[REG3a:[0-9]+]]
>>>>> +; CHECK:    sdc1    $f[[REG3b]], 0(${{[0-9]+}})
>>>>> +; CHECK:    .end    d1
>>>>> +  ret void
>>>>> +}
>>>>> +
>>>>> +attributes #0 = { nounwind "less-precise-fpmad"="false"
>>>>> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
>>>>> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
>>>>> "stack-protector-buffer-size"="8" "unsafe-fp-math"="false"
>>>>> "use-soft-float"="false" }
>>>>> +
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>
>>
>

--
http://www.nuanti.com
the browser experts





More information about the llvm-commits mailing list