[llvm-commits] [llvm] r159203 - in /llvm/trunk: lib/Target/ARM/ARMAsmPrinter.cpp lib/Target/CellSPU/SPUAsmPrinter.cpp lib/Target/Hexagon/HexagonAsmPrinter.cpp lib/Target/MBlaze/MBlazeAsmPrinter.cpp lib/Target/NVPTX/NVPTXAsmPrinter.cpp lib/Target/Po...
Jack Carter
Jack.Carter at imgtec.com
Mon Jul 15 11:52:05 PDT 2013
Aaron,
Thanks for catching the omission of the call to the generic AsmPrinter::PrintAsmOperand(). Better late than never.
What I don't understand is why a switch statement with only the default filled bad? It is a template for future additions and any compiler worth it's salt will compile it away.
Jack
________________________________________
From: aaron.ballman at gmail.com [aaron.ballman at gmail.com] on behalf of Aaron Ballman [aaron at aaronballman.com]
Sent: Tuesday, July 09, 2013 1:31 PM
To: Jack Carter; Richard Osborne
Cc: llvm-commits
Subject: Re: [llvm-commits] [llvm] r159203 - in /llvm/trunk: lib/Target/ARM/ARMAsmPrinter.cpp lib/Target/CellSPU/SPUAsmPrinter.cpp lib/Target/Hexagon/HexagonAsmPrinter.cpp lib/Target/MBlaze/MBlazeAsmPrinter.cpp lib/Target/NVPTX/NVPTXAsmPrinter.cpp lib/Target/Po...
I realize this commit is ancient, but I found something that was kind
of suspect while doing some code spelunking. This code has some
incorrect indentation, where it is indented at the same level as a
secondary if statement, but it should not be. So when ExtraCode[0] !=
0, the printOperand call is never made in two places (detailed below).
What's more, this code adds switch statements with no case labels
(just a default).
I've attached a patch that I believe is what the original patch was
intending, but since I don't understand the original intent, I'm
looking for eyeballs.
Thanks!
~Aaron
On Tue, Jun 26, 2012 at 9:49 AM, Jack Carter <jcarter at mips.com> wrote:
> Author: jacksprat
> Date: Tue Jun 26 08:49:27 2012
> New Revision: 159203
>
> URL: http://llvm.org/viewvc/llvm-project?rev=159203&view=rev
> Log:
> There are a number of generic inline asm operand modifiers that
> up to r158925 were handled as processor specific. Making them
> generic and putting tests for these modifiers in the CodeGen/Generic
> directory caused a number of targets to fail.
>
> This commit addresses that problem by having the targets call
> the generic routine for generic modifiers that they don't currently
> have explicit code for.
>
> For now only generic print operands 'c' and 'n' are supported.vi
>
>
> Affected files:
>
> test/CodeGen/Generic/asm-large-immediate.ll
> lib/Target/PowerPC/PPCAsmPrinter.cpp
> lib/Target/NVPTX/NVPTXAsmPrinter.cpp
> lib/Target/ARM/ARMAsmPrinter.cpp
> lib/Target/XCore/XCoreAsmPrinter.cpp
> lib/Target/X86/X86AsmPrinter.cpp
> lib/Target/Hexagon/HexagonAsmPrinter.cpp
> lib/Target/CellSPU/SPUAsmPrinter.cpp
> lib/Target/Sparc/SparcAsmPrinter.cpp
> lib/Target/MBlaze/MBlazeAsmPrinter.cpp
> lib/Target/Mips/MipsAsmPrinter.cpp
>
> MSP430 isn't represented because it did not even run with
> the long existing 'c' modifier and it was not apparent what
> needs to be done to get it inline asm ready.
>
> Contributer: Jack Carter
>
>
> Modified:
> llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp
> llvm/trunk/lib/Target/CellSPU/SPUAsmPrinter.cpp
> llvm/trunk/lib/Target/Hexagon/HexagonAsmPrinter.cpp
> llvm/trunk/lib/Target/MBlaze/MBlazeAsmPrinter.cpp
> llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
> llvm/trunk/lib/Target/PowerPC/PPCAsmPrinter.cpp
> llvm/trunk/lib/Target/Sparc/SparcAsmPrinter.cpp
> llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp
> llvm/trunk/lib/Target/XCore/XCoreAsmPrinter.cpp
> llvm/trunk/test/CodeGen/Generic/asm-large-immediate.ll
>
> Modified: llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -422,7 +422,9 @@
> if (ExtraCode[1] != 0) return true; // Unknown modifier.
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNum, AsmVariant, ExtraCode, O);
> case 'a': // Print as a memory address.
> if (MI->getOperand(OpNum).isReg()) {
> O << "["
>
> Modified: llvm/trunk/lib/Target/CellSPU/SPUAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/CellSPU/SPUAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/CellSPU/SPUAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/CellSPU/SPUAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -301,7 +301,9 @@
> if (ExtraCode[1] != 0) return true; // Unknown modifier.
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> case 'L': // Write second word of DImode reference.
> // Verify that this operand has two consecutive registers.
> if (!MI->getOperand(OpNo).isReg() ||
>
> Modified: llvm/trunk/lib/Target/Hexagon/HexagonAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Hexagon/HexagonAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/Hexagon/HexagonAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/Hexagon/HexagonAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -133,7 +133,9 @@
> if (ExtraCode[1] != 0) return true; // Unknown modifier.
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, OS);
> case 'c': // Don't print "$" before a global var name or constant.
> // Hexagon never has a prefix.
> printOperand(MI, OpNo, OS);
>
> Modified: llvm/trunk/lib/Target/MBlaze/MBlazeAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/MBlaze/MBlazeAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/MBlaze/MBlazeAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/MBlaze/MBlazeAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -200,7 +200,13 @@
> unsigned AsmVariant,const char *ExtraCode, raw_ostream &O) {
> // Does this asm operand have a single letter operand modifier?
> if (ExtraCode && ExtraCode[0])
> - return true; // Unknown modifier.
> + if (ExtraCode[1] != 0) return true; // Unknown modifier.
> +
> + switch (ExtraCode[0]) {
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> + }
Here.
>
> printOperand(MI, OpNo, O);
> return false;
>
> Modified: llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -1914,7 +1914,9 @@
> if (ExtraCode[1] != 0) return true; // Unknown modifier.
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> case 'r':
> break;
> }
>
> Modified: llvm/trunk/lib/Target/PowerPC/PPCAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/PowerPC/PPCAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/PowerPC/PPCAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -248,7 +248,9 @@
> if (ExtraCode[1] != 0) return true; // Unknown modifier.
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> case 'c': // Don't print "$" before a global var name or constant.
> break; // PPC never has a prefix.
> case 'L': // Write second word of DImode reference.
>
> Modified: llvm/trunk/lib/Target/Sparc/SparcAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Sparc/SparcAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/Sparc/SparcAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/Sparc/SparcAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -187,7 +187,9 @@
> if (ExtraCode[1] != 0) return true; // Unknown modifier.
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> case 'r':
> break;
> }
>
> Modified: llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -407,7 +407,9 @@
> const MachineOperand &MO = MI->getOperand(OpNo);
>
> switch (ExtraCode[0]) {
> - default: return true; // Unknown modifier.
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> case 'a': // This is an address. Currently only 'i' and 'r' are expected.
> if (MO.isImm()) {
> O << MO.getImm();
>
> Modified: llvm/trunk/lib/Target/XCore/XCoreAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/XCore/XCoreAsmPrinter.cpp?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/XCore/XCoreAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/XCore/XCoreAsmPrinter.cpp Tue Jun 26 08:49:27 2012
> @@ -260,7 +260,17 @@
> bool XCoreAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
> unsigned AsmVariant,const char *ExtraCode,
> raw_ostream &O) {
> - printOperand(MI, OpNo, O);
> + // Does this asm operand have a single letter operand modifier?
> + if (ExtraCode && ExtraCode[0])
> + if (ExtraCode[1] != 0) return true; // Unknown modifier.
> +
> + switch (ExtraCode[0]) {
> + default:
> + // See if this is a generic print operand
> + return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
> + }
Here.
> +
> +printOperand(MI, OpNo, O);
> return false;
> }
>
>
> Modified: llvm/trunk/test/CodeGen/Generic/asm-large-immediate.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Generic/asm-large-immediate.ll?rev=159203&r1=159202&r2=159203&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/Generic/asm-large-immediate.ll (original)
> +++ llvm/trunk/test/CodeGen/Generic/asm-large-immediate.ll Tue Jun 26 08:49:27 2012
> @@ -1,8 +1,5 @@
> ; RUN: llc < %s | FileCheck %s
>
> -; FIXME: Seek around r158932 to r158946.
> -; XFAIL: powerpc
> -
> define void @test() {
> entry:
> ; CHECK: /* result: 68719476738 */
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list