[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:59:40 PDT 2013
Richard,
If the original patch causes problems with XCore and MBlaze, that indicates to me that those targets are lacking sufficient tests in testsuite. I should not have been able to pass make check.
Jack
________________________________________
From: Richard Osborne [richard at xmos.com]
Sent: Monday, July 15, 2013 8:53 AM
To: Aaron Ballman
Cc: Jack Carter; 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...
Hi Aaron,
Thanks for pointing this out - it looks like r159203 introduces problems
with inline assembly in the XCore backend (and I assume the MBlaze
backend also). However I don't think your fix restores the old behavior.
In particular before r159203 in the case (!ExtraCode || !ExtraCode[0])
it would call printOperand(), but with your patch it will call
PrintAsmOperand which doesn't handle this case and returns true
indicating an erroneous operand. I think the right behavior is something
along line of:
if (ExtraCode && ExtraCode[0]) {
return AsmPrinter::PrintAsmOperand(MI, OpNo, AsmVariant, ExtraCode, O);
}
printOperand(MI, OpNo, O);
return false;
This change could also do with a testcases to check that inline asm
calls with a standard operand modifier (e.g. 'c') and with no operand
modifier give the right output.
Regards,
Richard
On 15/07/13 14:44, Aaron Ballman wrote:
> Ping?
>
> On Tue, Jul 9, 2013 at 4:31 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> 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
--
Richard Osborne | XMOS
http://www.xmos.com
More information about the llvm-commits
mailing list