[llvm-commits] [llvm] r155190 - /llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp

[Gabor Greif]

On 4/20/12, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> Hi Gabor,
>
> Do these new failures look related to your change?
> http://lab.llvm.org:8011/builders/llvm-i686-debian/builds/2782
> http://lab.llvm.org:8011/builders/llvm-x86_64-linux/builds/4324

Very much :-(

I was about to post this to the list:

---------------------------------------------------------------------------------------
Following up on this
<http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120416/141307.html>.

Something has activated this code and now the matching
of the <LOAD (ADD (X, Lo(G)))> pattern is triggered.

As expected, the following patch is needed to get past
the getZExtValue assert:


Index: lib/Target/PowerPC/PPCISelLowering.cpp
===================================================================
--- lib/Target/PowerPC/PPCISelLowering.cpp      (revision 155190)
+++ lib/Target/PowerPC/PPCISelLowering.cpp      (working copy)
@@ -902,7 +902,7 @@
       return true; // [r+i]
     } else if (N.getOperand(1).getOpcode() == PPCISD::Lo) {
       // Match LOAD (ADD (X, Lo(G))).
-      assert(!cast<ConstantSDNode>(N.getOperand(1).getOperand(0))->getZExtValue()
+      assert(!dyn_cast<ConstantSDNode>(N.getOperand(1).getOperand(0))
              && "Cannot handle constant offsets yet!");
       Disp = N.getOperand(1).getOperand(0);  // The global address.
       assert(Disp.getOpcode() == ISD::TargetGlobalAddress ||
@@ -1015,7 +1015,7 @@
       return true; // [r+i]
     } else if (N.getOperand(1).getOpcode() == PPCISD::Lo) {
       // Match LOAD (ADD (X, Lo(G))).
-      assert(!cast<ConstantSDNode>(N.getOperand(1).getOperand(0))->getZExtValue()
+      assert(!dyn_cast<ConstantSDNode>(N.getOperand(1).getOperand(0))
              && "Cannot handle constant offsets yet!");
       Disp = N.getOperand(1).getOperand(0);  // The global address.
       assert(Disp.getOpcode() == ISD::TargetGlobalAddress ||


After this, the assembly appears like this:

...
.Ltmp26:
	addis 3, 17, ha16(.L.str-.L0$pb)
	la 28, lo16(.L.str-.L0$pb)(3)
...

Alas, the GNU binutils assembler chokes on it with:
...
/tmp/ispb_hook-AJJRVu.s:116: Error: syntax error; found `(', expected `,'
/tmp/ispb_hook-AJJRVu.s:116: Error: junk at end of line: `(.L.str-.L0$pb)'
/tmp/ispb_hook-AJJRVu.s:117: Error: junk at end of line: `(3)'
...

It appears to me that the Apple asm syntax is being emitted, but
the Linux/IBM syntax is different and "@ha" and "@l" should be used
as a prefix.

What makes me nervous is that this part of the code has been dormant
for so much time and the "@ha", "@l" prefixed do not show up.

Can somebody tell me what is going on?

Cheers and thanks,

	Gabor
---------------------------------------------------------------------------------------

But it looks like I have to back out first.

Still, hope that somebody can explain me what was wrong with my patch
in the first place. I must be misunderstanding something.

Cheers,

     Gabor

>
> --
> Timur
>
> On Fri, Apr 20, 2012 at 12:58 PM, Gabor Greif <ggreif at gmail.com> wrote:
>> Author: ggreif
>> Date: Fri Apr 20 03:58:49 2012
>> New Revision: 155190
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=155190&view=rev
>> Log:
>> fix obviously bogus (IMO) operand index of the load in asserts
>> (load only has one operand) and smuggle in some whitespace changes too
>>
>> NB: I am obviously testing the water here, and believe that the unguarded
>>    cast is still wrong, but why is the getZExtValue of the load's operand
>>    tested against zero here? Any review is appreciated.
>>
>> Modified:
>>    llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
>>
>> Modified: llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp?rev=155190&r1=155189&r2=155190&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp (original)
>> +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp Fri Apr 20 03:58:49
>> 2012
>> @@ -902,7 +902,7 @@
>>       return true; // [r+i]
>>     } else if (N.getOperand(1).getOpcode() == PPCISD::Lo) {
>>       // Match LOAD (ADD (X, Lo(G))).
>> -
>> assert(!cast<ConstantSDNode>(N.getOperand(1).getOperand(1))->getZExtValue()
>> +
>>  assert(!cast<ConstantSDNode>(N.getOperand(1).getOperand(0))->getZExtValue()
>>              && "Cannot handle constant offsets yet!");
>>       Disp = N.getOperand(1).getOperand(0);  // The global address.
>>       assert(Disp.getOpcode() == ISD::TargetGlobalAddress ||
>> @@ -1006,7 +1006,7 @@
>>   if (N.getOpcode() == ISD::ADD) {
>>     short imm = 0;
>>     if (isIntS16Immediate(N.getOperand(1), imm) && (imm & 3) == 0) {
>> -      Disp =  DAG.getTargetConstant(((int)imm & 0xFFFF) >> 2, MVT::i32);
>> +      Disp = DAG.getTargetConstant(((int)imm & 0xFFFF) >> 2, MVT::i32);
>>       if (FrameIndexSDNode *FI =
>> dyn_cast<FrameIndexSDNode>(N.getOperand(0))) {
>>         Base = DAG.getTargetFrameIndex(FI->getIndex(), N.getValueType());
>>       } else {
>> @@ -1015,7 +1015,7 @@
>>       return true; // [r+i]
>>     } else if (N.getOperand(1).getOpcode() == PPCISD::Lo) {
>>       // Match LOAD (ADD (X, Lo(G))).
>> -
>> assert(!cast<ConstantSDNode>(N.getOperand(1).getOperand(1))->getZExtValue()
>> +
>>  assert(!cast<ConstantSDNode>(N.getOperand(1).getOperand(0))->getZExtValue()
>>              && "Cannot handle constant offsets yet!");
>>       Disp = N.getOperand(1).getOperand(0);  // The global address.
>>       assert(Disp.getOpcode() == ISD::TargetGlobalAddress ||
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>