llvm dev - Jun 2014 - [LLVMdev] [llvm] r210424 - Revert "Do materialize for floating point"

Why are you reverting patches for any area that you have no 
authorization for ?

No build was broken. This patch is fine.

I am authorized to check in to the Mips area and Daniel is the 
maintainer for that area.


On 06/08/2014 02:13 AM, Alp Toker wrote:
> Author: alp
> Date: Sun Jun  8 04:13:42 2014
> New Revision: 210424
>
> URL: http://llvm.org/viewvc/llvm-project?rev=210424&view=rev
> Log:
> Revert "Do materialize for floating point"
>
> 1) The commit was made despite profound lack of understanding:
>
>     "I did not understand the comment about using dyn_cast instead of
isa. I will
>     commit as is and make the update after. You can explain what you meant
to me."
>
>     Commit first, understand later isn't OK.
>
> 2) Review comments were simply ignored:
>
>     "Can you edit the summary to describe what the patch is for? It
appears to be
>     a list of commits at the moment."
>
> 3) The patch got LGTM'd off-list without any indication of readiness.
>
> 4) The public mailing list was excluded from patch review so all of this
was
>     hidden from the community.
>
> This reverts commit r210414.
>
> Removed:
>      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=210424&r1=210423&r2=210424&view=diff
>
=============================================================================>
--- llvm/trunk/lib/Target/Mips/MipsFastISel.cpp (original)
> +++ llvm/trunk/lib/Target/Mips/MipsFastISel.cpp Sun Jun  8 04:13:42 2014
> @@ -167,14 +167,9 @@ bool MipsFastISel::EmitStore(MVT VT, uns
>     //
>     // more cases will be handled here in following patches.
>     //
> -  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
> +  if (VT != MVT::i32)
>       return false;
> +  EmitInstStore(Mips::SW, SrcReg, Addr.Base.Reg, Addr.Offset);
>     return true;
>   }
>
> @@ -234,22 +229,6 @@ 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;
>   }
>
>
> Removed: 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=210423&view=auto
>
=============================================================================>
--- llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll (original)
> +++ llvm/trunk/test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll (removed)
> @@ -1,39 +0,0 @@
> -; 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" }
> -
>

On 8 June 2014 05:43, Reed Kotler <rkotler at mips.com>
wrote:
> Why are you reverting patches for any area that you have no authorization
> for ?
>
> No build was broken. This patch is fine.
>
> I am authorized to check in to the Mips area and Daniel is the maintainer
> for that area.
Hi Reed,

I hadn't noticed this patch until now, so this is mostly about
protocol, not my opinion about the patch.

Code ownership in llvm is a statement more about responsibility than
actual authority. Think of it as owning the code review, not the code
proper. It is the code owner responsibility to make sure the reviews
run correctly for a particular section, but there is no special
permission for cutting out others from the review process or other
shortcuts.

In this particular case it looks like some review feedback was
ignored, at least in the public list. For example, if the isa x
dyn_cast issue is a pattern over all of the llvm code base. So if a
misuse was noticed during code review, it should be handled. If
outside of the list it was decided that the original patch was
actually correct, it is good practice to say something on commit
message about it.

Same goes for having a commit message more in line with what we do in
llvm. Looking at it now I see that it has paragraphs that are not even
full sentences ("formatting"). What is being formatted? Why is that
not an independent patch?

So it does look like we will be better off with the patch reverted and
recommitted, but then with a better explanation of what is going on
and the llvm wide issues (isa X dyn_cast, incremental commits, etc)
fixed.

Cheers,
Rafael

> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu]
> On Behalf Of Rafael Espíndola
> Sent: 08 June 2014 15:03
> To: Reed Kotler
> Cc: LLVM Developers Mailing List
> Subject: Re: [LLVMdev] [llvm] r210424 - Revert "Do materialize for
floating
> point"
> 
> On 8 June 2014 05:43, Reed Kotler <rkotler at mips.com> wrote:
> > Why are you reverting patches for any area that you have no
> > authorization for ?
> >
> > No build was broken. This patch is fine.
> >
> > I am authorized to check in to the Mips area and Daniel is the
> > maintainer for that area.
> 
> Hi Reed,
> 
> I hadn't noticed this patch until now, so this is mostly about
protocol, not my
> opinion about the patch.
> 
> Code ownership in llvm is a statement more about responsibility than actual
> authority. Think of it as owning the code review, not the code proper. It
is
> the code owner responsibility to make sure the reviews run correctly for a
> particular section, but there is no special permission for cutting out
others
> from the review process or other shortcuts.
I agree. I've clarified this misconception in more detail on the
llvm-commits thread but to re-iterate the main point on this list: The developer
policy distinguishes between developers who maintain/contributed code and those
who don't/didn't to guide decisions on whether on-list pre-commit review
is required but that's as far as it goes. There's no concept of
authorized maintainers for an area.
> In this particular case it looks like some review feedback was ignored, at
least
> in the public list. For example, if the isa x dyn_cast issue is a pattern
over all
> of the llvm code base. So if a misuse was noticed during code review, it
> should be handled. If outside of the list it was decided that the original
patch
> was actually correct, it is good practice to say something on commit
message
> about it.
For what it's worth, it turns out that I'd asked Reed to make a change
that he'd already made way back on the 1st of May which confused him. I
discovered this yesterday (after replying to the llvm-commits thread) when I
went to explain the request in more detail. That said, I agree with the
'understand first, commit later' comments that were raised on the
llvm-commits thread. The commit shouldn't have happened until I had
clarified the request, discovered that it was already done, and replied to the
review.
> Same goes for having a commit message more in line with what we do in
> llvm. Looking at it now I see that it has paragraphs that are not even full
> sentences ("formatting"). What is being formatted? Why is that
not an
> independent patch?
> 
> So it does look like we will be better off with the patch reverted and
> recommitted, but then with a better explanation of what is going on and the
> llvm wide issues (isa X dyn_cast, incremental commits, etc) fixed.
> 
> Cheers,
> Rafael
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev