[llvm] r215155 - fix materialization of one bit constants and global values which are accessed through
Reed Kotler
rkotler at mips.com
Tue Aug 12 07:13:34 PDT 2014
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.
When more of fast-isel for Mips is pushed upstream, I think that a
simpler test case will be possible.
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.
So, if this regressed for some reason, our build bots will go seriously red.
> -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
More information about the llvm-commits
mailing list