[llvm] r215682 - [FastISel][ARM] Fall-back to constant pool loads when materializing an i32 constant.

[Jim Grosbach]

Looks great. Thanks!

-Jim

> On Aug 15, 2014, at 10:38 AM, Juergen Ributzka <juergen at apple.com> wrote:
> 
> 
> On Aug 15, 2014, at 9:41 AM, Jim Grosbach <grosbach at apple.com> wrote:
> 
>>> 
>>> On Aug 14, 2014, at 4:29 PM, Juergen Ributzka <juergen at apple.com> wrote:
>>> 
>>> Author: ributzka
>>> Date: Thu Aug 14 18:29:49 2014
>>> New Revision: 215682
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=215682&view=rev
>>> Log:
>>> [FastISel][ARM] Fall-back to constant pool loads when materializing an i32 constant.
>>> 
>>> FastEmit_i won't always succeed to materialize an i32 constant and just fail.
>>> This would trigger a fall-back to SelectionDAG, which is really not necessary.
>>> 
>>> This fix will first fall-back to a constant pool load to materialize the constant
>>> before giving up for good.
>>> 
>>> This fixes <rdar://problem/18022633>.
>>> 
>>> Modified:
>>>   llvm/trunk/lib/Target/ARM/ARMFastISel.cpp
>>>   llvm/trunk/test/CodeGen/ARM/fast-isel-mvn.ll
>>> 
>>> Modified: llvm/trunk/lib/Target/ARM/ARMFastISel.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMFastISel.cpp?rev=215682&r1=215681&r2=215682&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/ARM/ARMFastISel.cpp (original)
>>> +++ llvm/trunk/lib/Target/ARM/ARMFastISel.cpp Thu Aug 14 18:29:49 2014
>>> @@ -547,7 +547,8 @@ unsigned ARMFastISel::ARMMaterializeInt(
>>>  }
>>> 
>>>  if (Subtarget->useMovt(*FuncInfo.MF))
>>> -    return FastEmit_i(VT, VT, ISD::Constant, CI->getZExtValue());
>>> +    if (FastEmit_i(VT, VT, ISD::Constant, CI->getZExtValue()))
>>> +      return true;
>> 
>> The function returns an unsigned containing the reg of the result, not true or false. This discards the result register The same holds for the few other places in there which return ‘false’, but thankfully that’s less harmful as that maps to zero, which is what the function is supposed to return on failure.
> 
> You beat me to it Jim. I just realized it this morning, but you were faster. Was a horrible think-o. I cleaned up the return ‘false’ too while I fixed this in r215727.
> 
>> 
>> There is an assert() in FastISel::FastEmit_ri_() which is what’s firing in the original testcase. Specifically,
>>  unsigned MaterialReg = FastEmit_i(ImmType, ImmType, ISD::Constant, Imm);       
>>  if (MaterialReg == 0) {                                                        
>>    // This is a bit ugly/slow, but failing here means falling out of            
>>    // fast-isel, which would be very slow.                                      
>>    IntegerType *ITy = IntegerType::get(FuncInfo.Fn->getContext(),               
>>                                              VT.getSizeInBits());               
>>    MaterialReg = getRegForValue(ConstantInt::get(ITy, Imm));                    
>>    assert (MaterialReg != 0 && "Unable to materialize imm.");                   
>>    if (MaterialReg == 0) return 0;                                              
>>  } 
>> 
>> Why is it not correct in the general case to remove that assert()? Teaching the ARM code to be smarter so we won’t hit the assert() is fine, but that’s not a root cause fix if the assert() itself is wrong. The line immediately following explicitly handles the case that the assert() is checking, so something seems very wrong here and this is where the fix should be made. If we want to improve the ARM codegen as well, that’s great, but that’s an optimization, not a correctness issue.
> 
> Yup, I will remove it. I guess it was placed there to help track down trivial materialization failures.
> 
>> 
>> 
>>> 
>>>  // Load from constant pool.  For now 32-bit only.
>>>  if (VT != MVT::i32)
>>> 
>>> Modified: llvm/trunk/test/CodeGen/ARM/fast-isel-mvn.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/fast-isel-mvn.ll?rev=215682&r1=215681&r2=215682&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/CodeGen/ARM/fast-isel-mvn.ll (original)
>>> +++ llvm/trunk/test/CodeGen/ARM/fast-isel-mvn.ll Thu Aug 14 18:29:49 2014
>>> @@ -106,3 +106,15 @@ entry:
>>>  call void @foo(i32 -2130706433)
>>>  ret void
>>> }
>>> +
>>> +; Load from constant pool.
>>> +define i32 @t10(i32 %a) {
>>> +; ARM-LABEL:   t10
>>> +; ARM:         ldr
>>> +; THUMB-LABEL: t10
>>> +; THUMB:       movw r1, #52257
>>> +; THUMB-NEXT:  movt r1, #35037
>>> +  %1 = xor i32 -1998730207, %a
>>> +  ret i32 %1
>>> +
>> How does this test the patch? There’s no constant pool reference here. This test passes both before and after this patch is applied.
> 
> Oops, I fixed this in r215733. MOVT was disabled for the ARM tests. There is no constant pool reference, because I only check for the load instruction itself - like the other tests do in this file.
> 
>> 
>>> }
>>> +
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> 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/20140815/823266d9/attachment.html>