[llvm] r215155 - fix materialization of one bit constants and global values which are accessed through

Eric Christopher echristo at gmail.com
Wed Aug 13 18:52:02 PDT 2014


On Tue Aug 12 2014 at 7:13:41 AM Reed Kotler <rkotler at mips.com> wrote:

> On 08/11/2014 11:54 PM, Eric Christopher wrote:
> > On Mon, Aug 11, 2014 at 2:32 AM, Daniel Sanders
> > <Daniel.Sanders-1AXoQHu6uovQT0dZR+AlfA at public.gmane.org> wrote:
> >> Hi Eric,
> >>
> >> I could be wrong, but I don't think it's possible to write a robust
> test case for the boolean materialization bug. My thinking on reading the
> patch was that the bug would occur when FastISel generates the constant as
> an i1, but SelectionDAG eventually consumes it expecting an target boolean
> (i32 zero extended from an i1[*]). As FastISel handles more of the IR, the
> places where this can occur will disappear.
> >>
> >
> > Seemed there was likely a testcase that the code came from of it came
> > from though? Something reduced that highlighted the problem for the
> > patch?
> >
>
> Quite a few test-suite tests will fail without this patch.
> I don't remember exactly how many but at least 20.
>
> I spent a little time trying to make a small make-check test case but
> was not successful but it's on my list of things to do. Someone on the
> list suggested on idea which I did not have a chance to try yet.
>
>
How about putting an assert where you'd do something and then using
bugpoint? Do you have an ETA on the testcase? Do you have a function where
this code should be emitted?


> When more of fast-isel for Mips is pushed upstream, I think that a
> simpler test case will be possible.
>
>
I don't see how this is relevant.


> This is a feature and thus there should be a make-check test for x86,
> ARM and PPC fast isel but I did not see a test case there either.
>
>
Fairly irrelevant.


> So, if this regressed for some reason, our build bots will go seriously
> red.
>
>
Then it should be tested :)

-eric


> > -eric
> >
> >> I suppose we could have a simple test for 'ldi 1' rather than 'ldi -1',
> however both are correct according to the IR and it follows that a more
> complete FastISel implementation should be able to use either. Having said
> that, I've just looked at the boundary between FastISel and SelectionDAG
> (in SelectionDAGISel::SelectAllBasicBlocks()) and I couldn't see anything
> that legalizes the types/values that FastISel used so perhaps there's an
> unwritten rule that says that FastISel must legalize in a similar way to
> SelectionDAG.
> >>
> >> [*] Sign-extended for MIPS32r6/MIPS64r6 floating-point comparisons but
> we can handle that later.
> >>
> >>> -----Original Message-----
> >>> From: llvm-commits-bounces-Tmj1lob9twqVc3sceRu5cw at public.gmane.org
> [mailto:llvm-commits-
> >>> bounces-Tmj1lob9twqVc3sceRu5cw at public.gmane.org] On Behalf Of Eric
> Christopher
> >>> Sent: 08 August 2014 23:20
> >>> To: Reed Kotler
> >>> Cc: llvm-commits-Tmj1lob9twqVc3sceRu5cw at public.gmane.org
> >>> Subject: Re: [llvm] r215155 - fix materialization of one bit constants
> and global
> >>> values which are accessed through
> >>>
> >>> Testcase for the boolean materialization?
> >>>
> >>> -eric
> >>>
> >>> On Thu, Aug 7, 2014 at 3:09 PM, Reed Kotler <
> rkotler-8NJIiSa5LzA at public.gmane.org> wrote:
> >>>> Author: rkotler
> >>>> Date: Thu Aug  7 17:09:01 2014
> >>>> New Revision: 215155
> >>>>
> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=215155&view=rev
> >>>> Log:
> >>>> fix materialization of one bit constants and global values which are
> accessed
> >>> through
> >>>> a base GOT entry.
> >>>>
> >>>> Summary:
> >>>> get tip of tree mips fast-isel to pass test-suite
> >>>>
> >>>> Two bugs were fixed:
> >>>>
> >>>> 1) one bit booleans were treated as 1 bit signed integers and so the
> literal
> >>> '1' could become sign extended.
> >>>> 2) mips uses got for pic but in certain cases, as with string
> constants for
> >>> example, many items can be referenced from the same got entry and this
> >>> case was not handled properly.
> >>>>
> >>>> Test Plan: test-suite
> >>>>
> >>>> Reviewers: dsanders
> >>>>
> >>>> Reviewed By: dsanders
> >>>>
> >>>> Subscribers: mcrosier
> >>>>
> >>>> Differential Revision: http://reviews.llvm.org/D4801
> >>>>
> >>>> Added:
> >>>>      llvm/trunk/test/CodeGen/Mips/Fast-ISel/loadstrconst.ll
> >>>> Modified:
> >>>>      llvm/trunk/lib/Target/Mips/MipsFastISel.cpp
> >>>>
> >>>> Modified: llvm/trunk/lib/Target/Mips/MipsFastISel.cpp
> >>>> URL: http://llvm.org/viewvc/llvm-
> >>> project/llvm/trunk/lib/Target/Mips/MipsFastISel.cpp?rev=
> 215155&r1=21515
> >>> 4&r2=215155&view=diff
> >>>>
> >>> ==========================================================
> >>> ====================
> >>>> --- llvm/trunk/lib/Target/Mips/MipsFastISel.cpp (original)
> >>>> +++ llvm/trunk/lib/Target/Mips/MipsFastISel.cpp Thu Aug  7 17:09:01
> 2014
> >>>> @@ -110,7 +110,7 @@ private:
> >>>>     }
> >>>>
> >>>>     MachineInstrBuilder EmitInstLoad(unsigned Opc, unsigned DstReg,
> >>>> -                                      unsigned MemReg, int64_t
> MemOffset) {
> >>>> +                                   unsigned MemReg, int64_t
> MemOffset) {
> >>>>       return EmitInst(Opc, DstReg).addReg(MemReg).addImm(MemOffset);
> >>>>     }
> >>>>
> >>>> @@ -353,15 +353,23 @@ unsigned MipsFastISel::MaterializeGV(con
> >>>>       return 0;
> >>>>     EmitInst(Mips::LW, DestReg).addReg(MFI-
> >>>> getGlobalBaseReg()).addGlobalAddress(
> >>>>         GV, 0, MipsII::MO_GOT);
> >>>> +  if ((GV->hasInternalLinkage() ||
> >>>> +       (GV->hasLocalLinkage() && !isa<Function>(GV)))) {
> >>>> +    unsigned TempReg = createResultReg(RC);
> >>>> +    EmitInst(Mips::ADDiu, TempReg).addReg(DestReg).addGlobalAddress(
> >>>> +        GV, 0, MipsII::MO_ABS_LO);
> >>>> +    DestReg = TempReg;
> >>>> +  }
> >>>>     return DestReg;
> >>>>   }
> >>>> +
> >>>>   unsigned MipsFastISel::MaterializeInt(const Constant *C, MVT VT) {
> >>>>     if (VT != MVT::i32 && VT != MVT::i16 && VT != MVT::i8 && VT !=
> MVT::i1)
> >>>>       return 0;
> >>>>     const TargetRegisterClass *RC = &Mips::GPR32RegClass;
> >>>>     const ConstantInt *CI = cast<ConstantInt>(C);
> >>>>     int64_t Imm;
> >>>> -  if (CI->isNegative())
> >>>> +  if ((VT != MVT::i1) && CI->isNegative())
> >>>>       Imm = CI->getSExtValue();
> >>>>     else
> >>>>       Imm = CI->getZExtValue();
> >>>>
> >>>> Added: llvm/trunk/test/CodeGen/Mips/Fast-ISel/loadstrconst.ll
> >>>> URL: http://llvm.org/viewvc/llvm-
> >>> project/llvm/trunk/test/CodeGen/Mips/Fast-
> >>> ISel/loadstrconst.ll?rev=215155&view=auto
> >>>>
> >>> ==========================================================
> >>> ====================
> >>>> --- llvm/trunk/test/CodeGen/Mips/Fast-ISel/loadstrconst.ll (added)
> >>>> +++ llvm/trunk/test/CodeGen/Mips/Fast-ISel/loadstrconst.ll Thu Aug  7
> >>> 17:09:01 2014
> >>>> @@ -0,0 +1,19 @@
> >>>> +; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel
> -fast-
> >>> isel-abort -mcpu=mips32r2 \
> >>>> +; RUN:     < %s | FileCheck %s
> >>>> +
> >>>> + at .str = private unnamed_addr constant [6 x i8] c"hello\00", align 1
> >>>> + at s = common global i8* null, align 4
> >>>> +
> >>>> +; Function Attrs: nounwind
> >>>> +define void @foo() #0 {
> >>>> +entry:
> >>>> +  store i8* getelementptr inbounds ([6 x i8]* @.str, i32 0, i32 0),
> i8** @s,
> >>> align 4
> >>>> +  ret void
> >>>> +; CHECK:        .ent    foo
> >>>> +; CHECK:        lw      $[[REG1:[0-9]+]], %got($.str)(${{[0-9]+}})
> >>>> +; CHECK:        addiu   ${{[0-9]+}}, $[[REG1]], %lo($.str)
> >>>> +
> >>>> +}
> >>>> +
> >>>> +attributes #0 = { nounwind "less-precise-fpmad"="false" "no-frame-
> >>> pointer-elim"="false" "no-infs-fp-math"="false"
> "no-nans-fp-math"="false"
> >>> "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-
> >>> float"="false" }
> >>>> +
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> llvm-commits mailing list
> >>>> llvm-commits-Tmj1lob9twqVc3sceRu5cw at public.gmane.org
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits-Tmj1lob9twqVc3sceRu5cw at public.gmane.org
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140814/2e0ed7af/attachment.html>


More information about the llvm-commits mailing list