Reed Kotler
2014-Jun-08 09:43 UTC
[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" } > - >
Rafael Espíndola
2014-Jun-08 14:03 UTC
[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. 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
Daniel Sanders
2014-Jun-09 09:12 UTC
[LLVMdev] [llvm] r210424 - Revert "Do materialize for floating point"
> -----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