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

[Jim Grosbach]

> 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.

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.


> 
>   // 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.

> }
> +
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits