[llvm-commits] [llvm] r155190 - /llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
Hal Finkel
hfinkel at anl.gov
Fri Apr 20 06:39:22 PDT 2012
Gabor,
To comment on your second question first, I'd imagine that the current
code is relatively fragile, and probably does default to Apple syntax.
We should correct these things as we find them.
Did running make test work on your system prior to committing the
change? Am I correct is saying that the asserts that you changed were
not the ones that were triggering in the regression tests? If so, that
seems very odd (because the regression tests were not asserting
previously).
-Hal
On Fri, 20 Apr 2012 13:36:59 +0200
Gabor Greif <ggreif at gmail.com> wrote:
> 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
> >
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
--
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list