<br><br><div class="gmail_quote">On Tue Aug 12 2014 at 7:13:41 AM Reed Kotler <<a href="mailto:rkotler@mips.com">rkotler@mips.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 08/11/2014 11:54 PM, Eric Christopher wrote:<br>
> On Mon, Aug 11, 2014 at 2:32 AM, Daniel Sanders<br>
> <<a href="mailto:Daniel.Sanders-1AXoQHu6uovQT0dZR%2BAlfA@public.gmane.org" target="_blank">Daniel.Sanders-<u></u>1AXoQHu6uovQT0dZR+AlfA@public.<u></u>gmane.org</a>> wrote:<br>
>> Hi Eric,<br>
>><br>
>> 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.<br>
>><br>
><br>
> Seemed there was likely a testcase that the code came from of it came<br>
> from though? Something reduced that highlighted the problem for the<br>
> patch?<br>
><br>
<br>
Quite a few test-suite tests will fail without this patch.<br>
I don't remember exactly how many but at least 20.<br>
<br>
I spent a little time trying to make a small make-check test case but<br>
was not successful but it's on my list of things to do. Someone on the<br>
list suggested on idea which I did not have a chance to try yet.<br>
<br></blockquote><div><br></div><div>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?</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
When more of fast-isel for Mips is pushed upstream, I think that a<br>
simpler test case will be possible.<br>
<br></blockquote><div><br></div><div>I don't see how this is relevant.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is a feature and thus there should be a make-check test for x86,<br>
ARM and PPC fast isel but I did not see a test case there either.<br>
<br></blockquote><div><br></div><div>Fairly irrelevant.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So, if this regressed for some reason, our build bots will go seriously red.<br>
<br></blockquote><div><br></div><div>Then it should be tested :)</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> -eric<br>
><br>
>> 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::<u></u>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.<br>
>><br>
>> [*] Sign-extended for MIPS32r6/MIPS64r6 floating-point comparisons but we can handle that later.<br>
>><br>
>>> -----Original Message-----<br>
>>> From: <a href="mailto:llvm-commits-bounces-Tmj1lob9twqVc3sceRu5cw@public.gmane.org" target="_blank">llvm-commits-bounces-<u></u>Tmj1lob9twqVc3sceRu5cw@public.<u></u>gmane.org</a> [mailto:<a href="mailto:llvm-commits-" target="_blank">llvm-commits-</a><br>
>>> <a href="mailto:bounces-Tmj1lob9twqVc3sceRu5cw@public.gmane.org" target="_blank">bounces-<u></u>Tmj1lob9twqVc3sceRu5cw@public.<u></u>gmane.org</a>] On Behalf Of Eric Christopher<br>
>>> Sent: 08 August 2014 23:20<br>
>>> To: Reed Kotler<br>
>>> Cc: <a href="mailto:llvm-commits-Tmj1lob9twqVc3sceRu5cw@public.gmane.org" target="_blank">llvm-commits-<u></u>Tmj1lob9twqVc3sceRu5cw@public.<u></u>gmane.org</a><br>
>>> Subject: Re: [llvm] r215155 - fix materialization of one bit constants and global<br>
>>> values which are accessed through<br>
>>><br>
>>> Testcase for the boolean materialization?<br>
>>><br>
>>> -eric<br>
>>><br>
>>> On Thu, Aug 7, 2014 at 3:09 PM, Reed Kotler <<a href="mailto:rkotler-8NJIiSa5LzA@public.gmane.org" target="_blank">rkotler-8NJIiSa5LzA@public.<u></u>gmane.org</a>> wrote:<br>
>>>> Author: rkotler<br>
>>>> Date: Thu Aug 7 17:09:01 2014<br>
>>>> New Revision: 215155<br>
>>>><br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215155&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=215155&view=rev</a><br>
>>>> Log:<br>
>>>> fix materialization of one bit constants and global values which are accessed<br>
>>> through<br>
>>>> a base GOT entry.<br>
>>>><br>
>>>> Summary:<br>
>>>> get tip of tree mips fast-isel to pass test-suite<br>
>>>><br>
>>>> Two bugs were fixed:<br>
>>>><br>
>>>> 1) one bit booleans were treated as 1 bit signed integers and so the literal<br>
>>> '1' could become sign extended.<br>
>>>> 2) mips uses got for pic but in certain cases, as with string constants for<br>
>>> example, many items can be referenced from the same got entry and this<br>
>>> case was not handled properly.<br>
>>>><br>
>>>> Test Plan: test-suite<br>
>>>><br>
>>>> Reviewers: dsanders<br>
>>>><br>
>>>> Reviewed By: dsanders<br>
>>>><br>
>>>> Subscribers: mcrosier<br>
>>>><br>
>>>> Differential Revision: <a href="http://reviews.llvm.org/D4801" target="_blank">http://reviews.llvm.org/D4801</a><br>
>>>><br>
>>>> Added:<br>
>>>> llvm/trunk/test/CodeGen/Mips/<u></u>Fast-ISel/loadstrconst.ll<br>
>>>> Modified:<br>
>>>> llvm/trunk/lib/Target/Mips/<u></u>MipsFastISel.cpp<br>
>>>><br>
>>>> Modified: llvm/trunk/lib/Target/Mips/<u></u>MipsFastISel.cpp<br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
>>> project/llvm/trunk/lib/Target/<u></u>Mips/MipsFastISel.cpp?rev=<u></u>215155&r1=21515<br>
>>> 4&r2=215155&view=diff<br>
>>>><br>
>>> ==============================<u></u>============================<br>
>>> ====================<br>
>>>> --- llvm/trunk/lib/Target/Mips/<u></u>MipsFastISel.cpp (original)<br>
>>>> +++ llvm/trunk/lib/Target/Mips/<u></u>MipsFastISel.cpp Thu Aug 7 17:09:01 2014<br>
>>>> @@ -110,7 +110,7 @@ private:<br>
>>>> }<br>
>>>><br>
>>>> MachineInstrBuilder EmitInstLoad(unsigned Opc, unsigned DstReg,<br>
>>>> - unsigned MemReg, int64_t MemOffset) {<br>
>>>> + unsigned MemReg, int64_t MemOffset) {<br>
>>>> return EmitInst(Opc, DstReg).addReg(MemReg).addImm(<u></u>MemOffset);<br>
>>>> }<br>
>>>><br>
>>>> @@ -353,15 +353,23 @@ unsigned MipsFastISel::MaterializeGV(<u></u>con<br>
>>>> return 0;<br>
>>>> EmitInst(Mips::LW, DestReg).addReg(MFI-<br>
>>>> getGlobalBaseReg()).<u></u>addGlobalAddress(<br>
>>>> GV, 0, MipsII::MO_GOT);<br>
>>>> + if ((GV->hasInternalLinkage() ||<br>
>>>> + (GV->hasLocalLinkage() && !isa<Function>(GV)))) {<br>
>>>> + unsigned TempReg = createResultReg(RC);<br>
>>>> + EmitInst(Mips::ADDiu, TempReg).addReg(DestReg).<u></u>addGlobalAddress(<br>
>>>> + GV, 0, MipsII::MO_ABS_LO);<br>
>>>> + DestReg = TempReg;<br>
>>>> + }<br>
>>>> return DestReg;<br>
>>>> }<br>
>>>> +<br>
>>>> unsigned MipsFastISel::MaterializeInt(<u></u>const Constant *C, MVT VT) {<br>
>>>> if (VT != MVT::i32 && VT != MVT::i16 && VT != MVT::i8 && VT != MVT::i1)<br>
>>>> return 0;<br>
>>>> const TargetRegisterClass *RC = &Mips::GPR32RegClass;<br>
>>>> const ConstantInt *CI = cast<ConstantInt>(C);<br>
>>>> int64_t Imm;<br>
>>>> - if (CI->isNegative())<br>
>>>> + if ((VT != MVT::i1) && CI->isNegative())<br>
>>>> Imm = CI->getSExtValue();<br>
>>>> else<br>
>>>> Imm = CI->getZExtValue();<br>
>>>><br>
>>>> Added: llvm/trunk/test/CodeGen/Mips/<u></u>Fast-ISel/loadstrconst.ll<br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
>>> project/llvm/trunk/test/<u></u>CodeGen/Mips/Fast-<br>
>>> ISel/loadstrconst.ll?rev=<u></u>215155&view=auto<br>
>>>><br>
>>> ==============================<u></u>============================<br>
>>> ====================<br>
>>>> --- llvm/trunk/test/CodeGen/Mips/<u></u>Fast-ISel/loadstrconst.ll (added)<br>
>>>> +++ llvm/trunk/test/CodeGen/Mips/<u></u>Fast-ISel/loadstrconst.ll Thu Aug 7<br>
>>> 17:09:01 2014<br>
>>>> @@ -0,0 +1,19 @@<br>
>>>> +; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-<br>
>>> isel-abort -mcpu=mips32r2 \<br>
>>>> +; RUN: < %s | FileCheck %s<br>
>>>> +<br>
>>>> +@.str = private unnamed_addr constant [6 x i8] c"hello\00", align 1<br>
>>>> +@s = common global i8* null, align 4<br>
>>>> +<br>
>>>> +; Function Attrs: nounwind<br>
>>>> +define void @foo() #0 {<br>
>>>> +entry:<br>
>>>> + store i8* getelementptr inbounds ([6 x i8]* @.str, i32 0, i32 0), i8** @s,<br>
>>> align 4<br>
>>>> + ret void<br>
>>>> +; CHECK: .ent foo<br>
>>>> +; CHECK: lw $[[REG1:[0-9]+]], %got($.str)(${{[0-9]+}})<br>
>>>> +; CHECK: addiu ${{[0-9]+}}, $[[REG1]], %lo($.str)<br>
>>>> +<br>
>>>> +}<br>
>>>> +<br>
>>>> +attributes #0 = { nounwind "less-precise-fpmad"="false" "no-frame-<br>
>>> pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false"<br>
>>> "stack-protector-buffer-size"=<u></u>"8" "unsafe-fp-math"="false" "use-soft-<br>
>>> float"="false" }<br>
>>>> +<br>
>>>><br>
>>>><br>
>>>> ______________________________<u></u>_________________<br>
>>>> llvm-commits mailing list<br>
>>>> <a href="mailto:llvm-commits-Tmj1lob9twqVc3sceRu5cw@public.gmane.org" target="_blank">llvm-commits-<u></u>Tmj1lob9twqVc3sceRu5cw@public.<u></u>gmane.org</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
>>> ______________________________<u></u>_________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits-Tmj1lob9twqVc3sceRu5cw@public.gmane.org" target="_blank">llvm-commits-<u></u>Tmj1lob9twqVc3sceRu5cw@public.<u></u>gmane.org</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div>