[PATCH] Workaround MSVC 32-bit miscompile of getCondCodeAction

[Matt Arsenault]

Use 32-bit types for the array instead of 64. This should
generally be better anyway.
    
In optimized + assert builds, I saw a failure when a
cond code / type combination that is never set was loading
a non-zero value and hitting the != Promote assert.
    
It turns out when loading the 64-bit value to do the shift,
the assembly loads the 2 32-bit halves from non-consecutive 
addresses. The address the second half of the loaded uint64_t 
doesn't include the offset of the array in the struct. Instead of being 
offset + 4, it's just + 4.
    
I'm not entirely sure why this wasn't observed before.
setCondCodeAction isn't heavily used by the in-tree targets,
and not with the higher valued vector SimpleValueTypes. Only
PPC is using one of the > 32 valued types, and that is probably
never used by anyone on a 32-bit MSVC compiled host.
    
I ran into this when upgrading LLVM versions, so I guess the
value loaded from the nonsense address happened to work out
before.

No test since I'm not really sure if / how it can be reproduced with the
current in tree targets, and it's not supposed to change anything.

Here is the assembly for getCondCodeAction (asserts in the function removed) 
plus my debugging notes if somebody wants to check if I'm crazy.

//LegalizeAction
//getCondCodeAction(ISD::CondCode CC, EVT VT) const {
 push        esi
 mov         esi,ecx            ; Save this
 mov         ecx,dword ptr [esp+0Ch] ; Load VT.SimpleTy
 push        edi                     ; Do the assert that this is a simplety
 cmp         ecx,0FFh
 jle         llvm::TargetLowering::getCondCodeAction+2Bh (0ED715Bh)
 push        295h
 push        offset string L"c:\\Users\\marsenau"...
 push        offset string L"isSimple() &&"...
 call        _wassert (1544800h)
 mov         ecx,dword ptr [esp+1Ch]
 add         esp,0Ch
 mov         eax,dword ptr [esp+0Ch] ; Load CC, Assert passed jumps here
 mov         edx,ecx                 ; Copy SimpleTy
 shr         edx,5                   ; SimpleTy >> 5
 and         ecx,1Fh                 ; SimpleTy & 31
 lea         edx,[edx+eax*2]         ; 2 * CC + (SimpleTy >> 5)
 add         ecx,ecx                 ; 2 * (SimpleTy & 31)
 mov         eax,dword ptr [esi+edx*8+3230h] ; load [this + edx * 8 + offsetof CondCodeActions] (0 as expected)
 mov         edx,dword ptr [esi+edx*8+4]     ; load [this + edx * 8 + Not the offset of CondCodeActions] (nonsense value)
                                             ; Should add offset + 4, but instead just adds 4
 call        _aullshr (15467F0h)             ; Calling shift with non-consecutive loaded 32-bit parts of 64-bit load
 and         eax,3
 pop         esi
 ret         0Ch


    /*
 * unsigned long long
 * __aullshr(unsigned long long Value, unsigned char Shift);
 *
 * Parameters:
 *   EDX:EAX - unsigned long long value to be shifted right
 *   CL      - number of bits to shift by
 * Registers:
 *   Destroys CL
 * Returns:
 *   EDX:EAX - shifted value
 */


http://llvm-reviews.chandlerc.com/D1568

Files:
  include/llvm/Target/TargetLowering.h

Index: include/llvm/Target/TargetLowering.h
===================================================================
--- include/llvm/Target/TargetLowering.h
+++ include/llvm/Target/TargetLowering.h
@@ -520,13 +520,12 @@
   LegalizeAction
   getCondCodeAction(ISD::CondCode CC, MVT VT) const {
     assert((unsigned)CC < array_lengthof(CondCodeActions) &&
-           (unsigned)VT.SimpleTy < sizeof(CondCodeActions[0])*4 &&
+           ((unsigned)VT.SimpleTy >> 4) < array_lengthof(CondCodeActions[0]) &&
            "Table isn't big enough!");
-    /// The lower 5 bits of the SimpleTy index into Nth 2bit set from the 64bit
-    /// value and the upper 27 bits index into the second dimension of the
-    /// array to select what 64bit value to use.
-    LegalizeAction Action = (LegalizeAction)
-      ((CondCodeActions[CC][VT.SimpleTy >> 5] >> (2*(VT.SimpleTy & 0x1F))) & 3);
+    // See setCondCodeAction for how this is encoded.
+    uint32_t Shift = 2 * (VT.SimpleTy & 0xF);
+    uint32_t Value = CondCodeActions[CC][VT.SimpleTy >> 4];
+    LegalizeAction Action = (LegalizeAction) ((Value >> Shift) & 0x3);
     assert(Action != Promote && "Can't promote condition code!");
     return Action;
   }
@@ -1016,13 +1015,12 @@
     assert(VT < MVT::LAST_VALUETYPE &&
            (unsigned)CC < array_lengthof(CondCodeActions) &&
            "Table isn't big enough!");
-    /// The lower 5 bits of the SimpleTy index into Nth 2bit set from the 64bit
-    /// value and the upper 27 bits index into the second dimension of the
-    /// array to select what 64bit value to use.
-    CondCodeActions[(unsigned)CC][VT.SimpleTy >> 5]
-      &= ~(uint64_t(3UL)  << (VT.SimpleTy & 0x1F)*2);
-    CondCodeActions[(unsigned)CC][VT.SimpleTy >> 5]
-      |= (uint64_t)Action << (VT.SimpleTy & 0x1F)*2;
+    /// The lower 5 bits of the SimpleTy index into Nth 2bit set from the 32-bit
+    /// value and the upper 27 bits index into the second dimension of the array
+    /// to select what 32-bit value to use.
+    uint32_t Shift = 2 * (VT.SimpleTy & 0xF);
+    CondCodeActions[CC][VT.SimpleTy >> 4] &= ~((uint32_t)0x3 << Shift);
+    CondCodeActions[CC][VT.SimpleTy >> 4] |= (uint32_t)Action << Shift;
   }
 
   /// If Opc/OrigVT is specified as being promoted, the promotion code defaults
@@ -1417,8 +1415,8 @@
   ///
   /// Because each CC action takes up 2 bits, we need to have the array size be
   /// large enough to fit all of the value types. This can be done by dividing
-  /// the MVT::LAST_VALUETYPE by 32 and adding one.
-  uint64_t CondCodeActions[ISD::SETCC_INVALID][(MVT::LAST_VALUETYPE / 32) + 1];
+  /// the MVT::LAST_VALUETYPE by 16 and adding one.
+  uint32_t CondCodeActions[ISD::SETCC_INVALID][(MVT::LAST_VALUETYPE / 16) + 1];
 
   ValueTypeActionImpl ValueTypeActions;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D1568.1.patch
Type: text/x-patch
Size: 2783 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130830/2c6a226e/attachment.bin>