On Tue, Apr 08, 2014 at 04:08:13PM +0400, Robert Khasanov wrote:> Hi Reid, > > Would you approve your patches r203146 and r202774 to be backported to > 3.4.1? They fix stability issues in x86 asm. >Hi Robert, I was able to merge r203146, but it used a c++11 feature: std::string::back() which I replaced with std::string::at(std::string::size() - 1). r202774 was not merged, because it is dependent on r198584 which adds the X86AsmParser::is32BitMode() function. I don't think we would need to backport all of r198584, maybe just the function and the Mode32Bit SubtargetFeature. Could you take a look and let me know what you want to do? Thanks, Tom> Thanks, > Robert > > понедельник, 7 апреля 2014 г. пользователь Tom Stellard написал: > > > Hi Robert, > > > > Can you ping the code owners about these patches. It might be good > > to write a separate email per code owner and cc the appropriate -commits > > list. > > > > Thanks, > > Tom > > > > On Wed, Apr 02, 2014 at 06:16:44PM +0400, Robert Khasanov wrote: > > > Hi Tom, > > > > > > I would like to nominate the following patches to be backported to 3.4.1 > > > > > > Clang: > > > 1. r204742 - Zinovy Nis <zinovy.nis at gmail.com <javascript:;>> - Fix an > > logic error in the > > > clang driver preventing crtfastmath.o from linking when -Ofast is used > > > without -ffast-math > > > > > > LLVM: > > > 1. r205067 - Akira Hatanaka <ahatanaka at apple.com <javascript:;>> - > > [x86] Fix printing of > > > register operands with q modifier > > > 2. r203581 - Hans Wennborg <hans at hanshq.net <javascript:;>> - X86: > > Don't generate 64-bit > > > movd after cmpneqsd in 32-bit mode (PR19059) > > > 3. r203146 - Reid Kleckner <reid at kleckner.net <javascript:;>> - MS asm: > > The initial dot in > > > struct access is optional > > > 4. r202774 - Reid Kleckner <reid at kleckner.net <javascript:;>> - MC: Fix > > Intel assembly > > > parser for [global + offset] > > > 5. r201507 - Craig Topper <craig.topper at gmail.com <javascript:;>> - Fix > > diassembler > > > handling of rex.b when mod=00/01/10 and bbb=101. Mod=00 should ignore the > > > base register entirely. Mod=01/10 should treat this as R13 plus > > > displacment. Fixes PR18860 > > > 6. r201126 - Robert Khasanov - Changed attributes of all gather > > intrinsics > > > from IntrReadMem to IntrReadArgMem as they access only memory based on > > > argument. > > > > > > Most patches fix different stable issues on X86 target. > > > > > > Thanks, > > > Robert > > > > > > > > > > > > 2014-03-26 20:10 GMT+04:00 Tom Stellard <tom at stellard.net <javascript:;> > > >: > > > > > > > Hi, > > > > > > > > We are now about halfway between the 3.4 and 3.5 releases, and I would > > > > like to start preparing for a 3.4.1 release. Here is my proposed > > release > > > > schedule: > > > > > > > > Mar 26 - April 9: Identify and backport additional bug fixes to the 3.4 > > > > branch. > > > > April 9 - April 18: Testing Phase > > > > April 18: 3.4.1 Release > > > > > > > > How you can help: > > > > > > > > - If you have any bug fixes you think should be included to 3.4.1, send > > > > me an email with the SVN revision in trunk and also cc the code owner > > > > and llvm-commits (or cfe-commits if it is a clang patch). > > > > > > > > - Start integrating the 3.4 branch into your project or OS distribution > > > > to and check for any issues. > > > > > > > > - Volunteer as a tester for the testing phase. > > > > > > > > Thank you, > > > > > > > > Tom > > > > _______________________________________________ > > > > LLVM Developers mailing list > > > > LLVMdev at cs.uiuc.edu <javascript:;> http://llvm.cs.uiuc.edu > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > > >
Hi Tom, 2014-04-09 3:07 GMT+04:00 Tom Stellard <tom at stellard.net>:> On Tue, Apr 08, 2014 at 04:08:13PM +0400, Robert Khasanov wrote: > > Hi Reid, > > > > Would you approve your patches r203146 and r202774 to be backported to > > 3.4.1? They fix stability issues in x86 asm. > > > > Hi Robert, > > I was able to merge r203146, but it used a c++11 feature: > std::string::back() which I replaced with > std::string::at(std::string::size() - 1). >That is fine!> > r202774 was not merged, because it is dependent on r198584 which adds the > X86AsmParser::is32BitMode() function. I don't think we would need to > backport all of r198584, maybe just the function and the Mode32Bit > SubtargetFeature. Could you take a look and let me know what you want > to do? >I looked into this. r198584 introduces 16-bit mode support. I think that just adding function and Mode32Bit SubtargetFeature is not good because it is not good isolated from another code in this patch. I suggest another solution for this. Line that contains X86AsmParser::is32BitMode() isn't new, it is just replaced from code - if (isa<MCSymbolRefExpr>(Disp)) { - if (!Info.IsVarDecl) { - unsigned RegNo - is64BitMode() ? X86::RBX : (is32BitMode() ? X86::EBX : X86::BX); to + if (isa<MCSymbolRefExpr>(Disp) && !Info.IsVarDecl) { + unsigned RegNo + is64BitMode() ? X86::RBX : (is32BitMode() ? X86::EBX : X86::BX); Before 16-bit mode introducing, unsigned RegNo is64BitMode() ? X86::RBX : (is32BitMode() ? X86::EBX : X86::BX); was unsigned RegNo = is64BitMode() ? X86::RBX : X86::EBX; I think it would be better to just change this expression to "old" one. See attached modified r202774 patch for release_34 branch. I checked, build and make check works with this patch. Nadav, Reid, is it good solution? Thanks, Robert> Thanks, > Tom > > > Thanks, > > Robert > > > > понедельник, 7 апреля 2014 г. пользователь Tom Stellard написал: > > > > > Hi Robert, > > > > > > Can you ping the code owners about these patches. It might be good > > > to write a separate email per code owner and cc the appropriate > -commits > > > list. > > > > > > Thanks, > > > Tom > > > > > > On Wed, Apr 02, 2014 at 06:16:44PM +0400, Robert Khasanov wrote: > > > > Hi Tom, > > > > > > > > I would like to nominate the following patches to be backported to > 3.4.1 > > > > > > > > Clang: > > > > 1. r204742 - Zinovy Nis <zinovy.nis at gmail.com <javascript:;>> - Fix > an > > > logic error in the > > > > clang driver preventing crtfastmath.o from linking when -Ofast is > used > > > > without -ffast-math > > > > > > > > LLVM: > > > > 1. r205067 - Akira Hatanaka <ahatanaka at apple.com <javascript:;>> - > > > [x86] Fix printing of > > > > register operands with q modifier > > > > 2. r203581 - Hans Wennborg <hans at hanshq.net <javascript:;>> - X86: > > > Don't generate 64-bit > > > > movd after cmpneqsd in 32-bit mode (PR19059) > > > > 3. r203146 - Reid Kleckner <reid at kleckner.net <javascript:;>> - MS > asm: > > > The initial dot in > > > > struct access is optional > > > > 4. r202774 - Reid Kleckner <reid at kleckner.net <javascript:;>> - MC: > Fix > > > Intel assembly > > > > parser for [global + offset] > > > > 5. r201507 - Craig Topper <craig.topper at gmail.com <javascript:;>> - > Fix > > > diassembler > > > > handling of rex.b when mod=00/01/10 and bbb=101. Mod=00 should > ignore the > > > > base register entirely. Mod=01/10 should treat this as R13 plus > > > > displacment. Fixes PR18860 > > > > 6. r201126 - Robert Khasanov - Changed attributes of all gather > > > intrinsics > > > > from IntrReadMem to IntrReadArgMem as they access only memory based > on > > > > argument. > > > > > > > > Most patches fix different stable issues on X86 target. > > > > > > > > Thanks, > > > > Robert > > > > > > > > > > > > > > > > 2014-03-26 20:10 GMT+04:00 Tom Stellard <tom at stellard.net<javascript:;> > > > >: > > > > > > > > > Hi, > > > > > > > > > > We are now about halfway between the 3.4 and 3.5 releases, and I > would > > > > > like to start preparing for a 3.4.1 release. Here is my proposed > > > release > > > > > schedule: > > > > > > > > > > Mar 26 - April 9: Identify and backport additional bug fixes to > the 3.4 > > > > > branch. > > > > > April 9 - April 18: Testing Phase > > > > > April 18: 3.4.1 Release > > > > > > > > > > How you can help: > > > > > > > > > > - If you have any bug fixes you think should be included to 3.4.1, > send > > > > > me an email with the SVN revision in trunk and also cc the code > owner > > > > > and llvm-commits (or cfe-commits if it is a clang patch). > > > > > > > > > > - Start integrating the 3.4 branch into your project or OS > distribution > > > > > to and check for any issues. > > > > > > > > > > - Volunteer as a tester for the testing phase. > > > > > > > > > > Thank you, > > > > > > > > > > Tom > > > > > _______________________________________________ > > > > > LLVM Developers mailing list > > > > > LLVMdev at cs.uiuc.edu <javascript:;> http://llvm.cs.uiuc.edu > > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140411/86429564/attachment.html> -------------- next part -------------- Index: lib/Target/X86/AsmParser/X86AsmParser.cpp ==================================================================--- lib/Target/X86/AsmParser/X86AsmParser.cpp (revision 205931) +++ lib/Target/X86/AsmParser/X86AsmParser.cpp (working copy) @@ -1181,16 +1181,23 @@ unsigned Scale, SMLoc Start, SMLoc End, unsigned Size, StringRef Identifier, InlineAsmIdentifierInfo &Info){ - if (isa<MCSymbolRefExpr>(Disp)) { - // If this is not a VarDecl then assume it is a FuncDecl or some other label - // reference. We need an 'r' constraint here, so we need to create register - // operand to ensure proper matching. Just pick a GPR based on the size of - // a pointer. - if (!Info.IsVarDecl) { - unsigned RegNo = is64BitMode() ? X86::RBX : X86::EBX; - return X86Operand::CreateReg(RegNo, Start, End, /*AddressOf=*/true, - SMLoc(), Identifier, Info.OpDecl); - } + // If this is not a VarDecl then assume it is a FuncDecl or some other label + // reference. We need an 'r' constraint here, so we need to create register + // operand to ensure proper matching. Just pick a GPR based on the size of + // a pointer. + if (isa<MCSymbolRefExpr>(Disp) && !Info.IsVarDecl) { + unsigned RegNo = is64BitMode() ? X86::RBX : X86::EBX; + return X86Operand::CreateReg(RegNo, Start, End, /*AddressOf=*/true, + SMLoc(), Identifier, Info.OpDecl); + } + + // We either have a direct symbol reference, or an offset from a symbol. The + // parser always puts the symbol on the LHS, so look there for size + // calculation purposes. + const MCBinaryExpr *BinOp = dyn_cast<MCBinaryExpr>(Disp); + bool IsSymRef + isa<MCSymbolRefExpr>(BinOp ? BinOp->getLHS() : Disp); + if (IsSymRef) { if (!Size) { Size = Info.Type * 8; // Size is in terms of bits in this context. if (Size) @@ -1371,7 +1378,7 @@ if (ParseIntelExpression(SM, End)) return 0; - const MCExpr *Disp; + const MCExpr *Disp = 0; if (const MCExpr *Sym = SM.getSym()) { // A symbolic displacement. Disp = Sym; @@ -1379,11 +1386,16 @@ RewriteIntelBracExpression(InstInfo->AsmRewrites, SM.getSymName(), ImmDisp, SM.getImm(), BracLoc, StartInBrac, End); - } else { - // An immediate displacement only. - Disp = MCConstantExpr::Create(SM.getImm(), getContext()); } + if (SM.getImm() || !Disp) { + const MCExpr *Imm = MCConstantExpr::Create(SM.getImm(), getContext()); + if (Disp) + Disp = MCBinaryExpr::CreateAdd(Disp, Imm, getContext()); + else + Disp = Imm; // An immediate displacement only. + } + // Parse struct field access. Intel requires a dot, but MSVC doesn't. MSVC // will in fact do global lookup the field name inside all global typedefs, // but we don't emulate that. Index: test/MC/X86/intel-syntax.s ==================================================================--- test/MC/X86/intel-syntax.s (revision 205931) +++ test/MC/X86/intel-syntax.s (working copy) @@ -584,3 +584,12 @@ fsubr ST(1) fdiv ST(1) fdivr ST(1) + +.bss +.globl _g0 +.text + +// CHECK: movq _g0, %rbx +// CHECK: movq _g0+8, %rcx +mov rbx, qword ptr [_g0] +mov rcx, qword ptr [_g0 + 8]
This patch has been merged. -Tom On Fri, Apr 11, 2014 at 02:04:11AM +0400, Robert Khasanov wrote:> Hi Tom, > > > 2014-04-09 3:07 GMT+04:00 Tom Stellard <tom at stellard.net>: > > > On Tue, Apr 08, 2014 at 04:08:13PM +0400, Robert Khasanov wrote: > > > Hi Reid, > > > > > > Would you approve your patches r203146 and r202774 to be backported to > > > 3.4.1? They fix stability issues in x86 asm. > > > > > > > Hi Robert, > > > > I was able to merge r203146, but it used a c++11 feature: > > std::string::back() which I replaced with > > std::string::at(std::string::size() - 1). > > > > That is fine! > > > > > > r202774 was not merged, because it is dependent on r198584 which adds the > > X86AsmParser::is32BitMode() function. I don't think we would need to > > backport all of r198584, maybe just the function and the Mode32Bit > > SubtargetFeature. Could you take a look and let me know what you want > > to do? > > > > I looked into this. r198584 introduces 16-bit mode support. I think that > just adding function and Mode32Bit SubtargetFeature is not good because it > is not good isolated from another code in this patch. > I suggest another solution for this. > Line that contains X86AsmParser::is32BitMode() isn't new, it is just > replaced from code > - if (isa<MCSymbolRefExpr>(Disp)) { > - if (!Info.IsVarDecl) { > - unsigned RegNo > - is64BitMode() ? X86::RBX : (is32BitMode() ? X86::EBX : X86::BX); > > to > > + if (isa<MCSymbolRefExpr>(Disp) && !Info.IsVarDecl) { > + unsigned RegNo > + is64BitMode() ? X86::RBX : (is32BitMode() ? X86::EBX : X86::BX); > > Before 16-bit mode introducing, > unsigned RegNo > is64BitMode() ? X86::RBX : (is32BitMode() ? X86::EBX : X86::BX); > was > unsigned RegNo = is64BitMode() ? X86::RBX : X86::EBX; > > I think it would be better to just change this expression to "old" one. > > See attached modified r202774 patch for release_34 branch. > I checked, build and make check works with this patch. > > Nadav, Reid, > is it good solution? > > Thanks, > > Robert > > > > Thanks, > > Tom > > > > > Thanks, > > > Robert > > > > > > понедельник, 7 апреля 2014 г. пользователь Tom Stellard написал: > > > > > > > Hi Robert, > > > > > > > > Can you ping the code owners about these patches. It might be good > > > > to write a separate email per code owner and cc the appropriate > > -commits > > > > list. > > > > > > > > Thanks, > > > > Tom > > > > > > > > On Wed, Apr 02, 2014 at 06:16:44PM +0400, Robert Khasanov wrote: > > > > > Hi Tom, > > > > > > > > > > I would like to nominate the following patches to be backported to > > 3.4.1 > > > > > > > > > > Clang: > > > > > 1. r204742 - Zinovy Nis <zinovy.nis at gmail.com <javascript:;>> - Fix > > an > > > > logic error in the > > > > > clang driver preventing crtfastmath.o from linking when -Ofast is > > used > > > > > without -ffast-math > > > > > > > > > > LLVM: > > > > > 1. r205067 - Akira Hatanaka <ahatanaka at apple.com <javascript:;>> - > > > > [x86] Fix printing of > > > > > register operands with q modifier > > > > > 2. r203581 - Hans Wennborg <hans at hanshq.net <javascript:;>> - X86: > > > > Don't generate 64-bit > > > > > movd after cmpneqsd in 32-bit mode (PR19059) > > > > > 3. r203146 - Reid Kleckner <reid at kleckner.net <javascript:;>> - MS > > asm: > > > > The initial dot in > > > > > struct access is optional > > > > > 4. r202774 - Reid Kleckner <reid at kleckner.net <javascript:;>> - MC: > > Fix > > > > Intel assembly > > > > > parser for [global + offset] > > > > > 5. r201507 - Craig Topper <craig.topper at gmail.com <javascript:;>> - > > Fix > > > > diassembler > > > > > handling of rex.b when mod=00/01/10 and bbb=101. Mod=00 should > > ignore the > > > > > base register entirely. Mod=01/10 should treat this as R13 plus > > > > > displacment. Fixes PR18860 > > > > > 6. r201126 - Robert Khasanov - Changed attributes of all gather > > > > intrinsics > > > > > from IntrReadMem to IntrReadArgMem as they access only memory based > > on > > > > > argument. > > > > > > > > > > Most patches fix different stable issues on X86 target. > > > > > > > > > > Thanks, > > > > > Robert > > > > > > > > > > > > > > > > > > > > 2014-03-26 20:10 GMT+04:00 Tom Stellard <tom at stellard.net<javascript:;> > > > > >: > > > > > > > > > > > Hi, > > > > > > > > > > > > We are now about halfway between the 3.4 and 3.5 releases, and I > > would > > > > > > like to start preparing for a 3.4.1 release. Here is my proposed > > > > release > > > > > > schedule: > > > > > > > > > > > > Mar 26 - April 9: Identify and backport additional bug fixes to > > the 3.4 > > > > > > branch. > > > > > > April 9 - April 18: Testing Phase > > > > > > April 18: 3.4.1 Release > > > > > > > > > > > > How you can help: > > > > > > > > > > > > - If you have any bug fixes you think should be included to 3.4.1, > > send > > > > > > me an email with the SVN revision in trunk and also cc the code > > owner > > > > > > and llvm-commits (or cfe-commits if it is a clang patch). > > > > > > > > > > > > - Start integrating the 3.4 branch into your project or OS > > distribution > > > > > > to and check for any issues. > > > > > > > > > > > > - Volunteer as a tester for the testing phase. > > > > > > > > > > > > Thank you, > > > > > > > > > > > > Tom > > > > > > _______________________________________________ > > > > > > LLVM Developers mailing list > > > > > > LLVMdev at cs.uiuc.edu <javascript:;> http://llvm.cs.uiuc.edu > > > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > > > > > > > > >> Index: lib/Target/X86/AsmParser/X86AsmParser.cpp > ==================================================================> --- lib/Target/X86/AsmParser/X86AsmParser.cpp (revision 205931) > +++ lib/Target/X86/AsmParser/X86AsmParser.cpp (working copy) > @@ -1181,16 +1181,23 @@ > unsigned Scale, SMLoc Start, SMLoc End, > unsigned Size, StringRef Identifier, > InlineAsmIdentifierInfo &Info){ > - if (isa<MCSymbolRefExpr>(Disp)) { > - // If this is not a VarDecl then assume it is a FuncDecl or some other label > - // reference. We need an 'r' constraint here, so we need to create register > - // operand to ensure proper matching. Just pick a GPR based on the size of > - // a pointer. > - if (!Info.IsVarDecl) { > - unsigned RegNo = is64BitMode() ? X86::RBX : X86::EBX; > - return X86Operand::CreateReg(RegNo, Start, End, /*AddressOf=*/true, > - SMLoc(), Identifier, Info.OpDecl); > - } > + // If this is not a VarDecl then assume it is a FuncDecl or some other label > + // reference. We need an 'r' constraint here, so we need to create register > + // operand to ensure proper matching. Just pick a GPR based on the size of > + // a pointer. > + if (isa<MCSymbolRefExpr>(Disp) && !Info.IsVarDecl) { > + unsigned RegNo = is64BitMode() ? X86::RBX : X86::EBX; > + return X86Operand::CreateReg(RegNo, Start, End, /*AddressOf=*/true, > + SMLoc(), Identifier, Info.OpDecl); > + } > + > + // We either have a direct symbol reference, or an offset from a symbol. The > + // parser always puts the symbol on the LHS, so look there for size > + // calculation purposes. > + const MCBinaryExpr *BinOp = dyn_cast<MCBinaryExpr>(Disp); > + bool IsSymRef > + isa<MCSymbolRefExpr>(BinOp ? BinOp->getLHS() : Disp); > + if (IsSymRef) { > if (!Size) { > Size = Info.Type * 8; // Size is in terms of bits in this context. > if (Size) > @@ -1371,7 +1378,7 @@ > if (ParseIntelExpression(SM, End)) > return 0; > > - const MCExpr *Disp; > + const MCExpr *Disp = 0; > if (const MCExpr *Sym = SM.getSym()) { > // A symbolic displacement. > Disp = Sym; > @@ -1379,11 +1386,16 @@ > RewriteIntelBracExpression(InstInfo->AsmRewrites, SM.getSymName(), > ImmDisp, SM.getImm(), BracLoc, StartInBrac, > End); > - } else { > - // An immediate displacement only. > - Disp = MCConstantExpr::Create(SM.getImm(), getContext()); > } > > + if (SM.getImm() || !Disp) { > + const MCExpr *Imm = MCConstantExpr::Create(SM.getImm(), getContext()); > + if (Disp) > + Disp = MCBinaryExpr::CreateAdd(Disp, Imm, getContext()); > + else > + Disp = Imm; // An immediate displacement only. > + } > + > // Parse struct field access. Intel requires a dot, but MSVC doesn't. MSVC > // will in fact do global lookup the field name inside all global typedefs, > // but we don't emulate that. > Index: test/MC/X86/intel-syntax.s > ==================================================================> --- test/MC/X86/intel-syntax.s (revision 205931) > +++ test/MC/X86/intel-syntax.s (working copy) > @@ -584,3 +584,12 @@ > fsubr ST(1) > fdiv ST(1) > fdivr ST(1) > + > +.bss > +.globl _g0 > +.text > + > +// CHECK: movq _g0, %rbx > +// CHECK: movq _g0+8, %rcx > +mov rbx, qword ptr [_g0] > +mov rcx, qword ptr [_g0 + 8]