[DAGCombiner][AMDGPU][Mips] Fold bitcast with volatile loads if the resulting load is legal for the target.

[craig.topper]

Summary

I'm not sure if this patch is correct or if it needs more qualifying somehow. Bitcast shouldn't change the size of the load so it should be ok? We already do something similar for stores. We'll change the type of a volatile store if the resulting store is Legal or Custom. I'm not sure we should be allowing Custom there...

I was playing around with converting X86 atomic loads/stores(except seq_cst) into regular volatile loads and stores during lowering. This would allow some special RMW isel patterns in X86InstrCompiler.td to be removed. But there's some floating point patterns in there that didn't work because we don't fold (f64 (bitconvert (i64 volatile load))) or (f32 (bitconvert (i32 volatile load))).

[arsenm]

I've never understood the type legalization policy for volatile. It's never made any sense to me why it would change how loads and stores to be legalized based on the type, so this seems like an improvement to me

[efriedma]

For volatile loads/stores, we should probably try to preserve the number and addresses of memory operations. If the source and destination types would lower to equivalent memory operations, like i32 vs. f32 on x86, the transform is fine. If they wouldn't, like f64 vs. i64 on x86-32, the transform is dubious; memory-mapped I/O can respond in strange ways if you don't use the right access width.

[craig.topper]

So the isOperationLegal check here should prevent (i64 (bitcast (f64 volatile load))) from being changed on x86-32. Do I need to make sure sure that i64 is legal to prevent (f64 (bitcast (i64 volatile load))) from being changed? The comments from r52254 where this check was originally added seems to suggest that its bad to increase the number of operations and that the number of operations needed for an illegal type is "undefined".

[efriedma]

I guess we could assume anyone using memory-mapped hardware will only do it with legal types, so reducing the number of operations for illegal types doesn't matter. But please add a comment here (and to the related store transform) explaining that.

[craig.topper]

Add comment

[efriedma]

LGTM

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitBITCAST(SDNode *N) {
 
   // (conv (conv x, t1), t2) -> (conv x, t2)
   if (N0.getOpcode() == ISD::BITCAST)
     return DAG.getBitcast(VT, N0.getOperand(0));
 
   // fold (conv (load x)) -> (load (conv*)x)
   // If the resultant load doesn't need a higher alignment than the original!
   if (ISD::isNormalLoad(N0.getNode()) && N0.hasOneUse() &&
-      // Do not change the width of a volatile load.
-      !cast<LoadSDNode>(N0)->isVolatile() &&
       // Do not remove the cast if the types differ in endian layout.
       TLI.hasBigEndianPartOrdering(N0.getValueType(), DAG.getDataLayout()) ==
           TLI.hasBigEndianPartOrdering(VT, DAG.getDataLayout()) &&
-      (!LegalOperations || TLI.isOperationLegal(ISD::LOAD, VT)) &&
+      // If the load is volatile, we only want to change the load type if the
+      // resulting load is legal. Otherwise we might increase the number of
+      // memory accesses. We don't care if the original type was legal or not
+      // as we assume software couldn't rely on the number of accesses of an
+      // illegal type.
+      ((!LegalOperations && !cast<LoadSDNode>(N0)->isVolatile()) ||
+       TLI.isOperationLegal(ISD::LOAD, VT)) &&
       TLI.isLoadBitCastBeneficial(N0.getValueType(), VT)) {
     LoadSDNode *LN0 = cast<LoadSDNode>(N0);
     unsigned OrigAlign = LN0->getAlignment();
 
     bool Fast = false;
     if (TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), VT,
                                LN0->getAddressSpace(), OrigAlign, &Fast) &&
         Fast) {
SDValue DAGCombiner::visitSTORE(SDNode *N) {
   SDValue Value = ST->getValue();
   SDValue Ptr   = ST->getBasePtr();
 
   // If this is a store of a bit convert, store the input value if the
   // resultant store does not need a higher alignment than the original.
   if (Value.getOpcode() == ISD::BITCAST && !ST->isTruncatingStore() &&
       ST->isUnindexed()) {
     EVT SVT = Value.getOperand(0).getValueType();
+    // If the store is volatile, we only want to change the store type if the
+    // resulting store is legal. Otherwise we might increase the number of
+    // memory accesses. We don't care if the original type was legal or not
+    // as we assume software couldn't rely on the number of accesses of an
+    // illegal type.
     if (((!LegalOperations && !ST->isVolatile()) ||
          TLI.isOperationLegal(ISD::STORE, SVT)) &&
         TLI.isStoreBitCastBeneficial(Value.getValueType(), SVT)) {
       unsigned OrigAlign = ST->getAlignment();
       bool Fast = false;
       if (TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), SVT,
                                  ST->getAddressSpace(), OrigAlign, &Fast) &&
           Fast) {

test/CodeGen/AMDGPU/copy-illegal-type.ll

 ; GCN: s_endpgm
 define amdgpu_kernel void @test_copy_v3i8_align1(<3 x i8> addrspace(1)* %out, <3 x i8> addrspace(1)* %in) nounwind {
   %val = load <3 x i8>, <3 x i8> addrspace(1)* %in, align 1
   store <3 x i8> %val, <3 x i8> addrspace(1)* %out, align 1
   ret void
 }
 
 ; FUNC-LABEL: {{^}}test_copy_v4i8_volatile_load:
-; GCN: {{buffer|flat}}_load_ubyte
-; GCN: {{buffer|flat}}_load_ubyte
-; GCN: {{buffer|flat}}_load_ubyte
-; GCN: {{buffer|flat}}_load_ubyte
+; GCN: {{buffer|flat}}_load_dword
 ; GCN: buffer_store_dword
 ; GCN: s_endpgm
 define amdgpu_kernel void @test_copy_v4i8_volatile_load(<4 x i8> addrspace(1)* %out, <4 x i8> addrspace(1)* %in) nounwind {
   %val = load volatile <4 x i8>, <4 x i8> addrspace(1)* %in, align 4
   store <4 x i8> %val, <4 x i8> addrspace(1)* %out, align 4
   ret void
 }
 

test/CodeGen/Mips/cconv/return-hard-fp128.ll

 
 define fp128 @retldouble() nounwind {
 entry:
         %0 = load volatile fp128, fp128* @fp128
         ret fp128 %0
 }
 
 ; ALL-LABEL: retldouble:
-; N32-DAG:           ld [[R2:\$[0-9]+]], %lo(fp128)([[R1:\$[0-9]+]])
+; N32-DAG:           ldc1 $f0, %lo(fp128)([[R1:\$[0-9]+]])
 ; N32-DAG:           addiu [[R3:\$[0-9]+]], [[R1]], %lo(fp128)
-; N32-DAG:           ld [[R4:\$[0-9]+]], 8([[R3]])
-; N32-DAG:           dmtc1 [[R2]], $f0
-; N32-DAG:           dmtc1 [[R4]], $f2
+; N32-DAG:           ldc1 $f2, 8([[R3]])
 
 ; N64-DAG:           lui [[R2:\$[0-9]+]], %highest(fp128)
-; N64-DAG:           ld [[R3:\$[0-9]+]], %lo(fp128)([[R2]])
-; N64-DAG:           ld [[R4:\$[0-9]+]], 8([[R2]])
-; N64-DAG:           dmtc1 [[R3]], $f0
-; N64-DAG:           dmtc1 [[R4]], $f2
+; N64-DAG:           ldc1 $f0, %lo(fp128)([[R2]])
+; N64-DAG:           ldc1 $f2, 8([[R2]])

test/CodeGen/Mips/cconv/return-hard-struct-f128.ll

 
 ; O32 generates different IR so we don't test it here. It returns the struct
 ; indirectly.
 
 ; Contrary to the N32/N64 ABI documentation, a struct containing a long double
 ; is returned in $f0, and $f1 instead of the usual $f0, and $f2. This is to
 ; match the de facto ABI as implemented by GCC.
 ; N32-DAG:        lui [[R1:\$[0-9]+]], %hi(struct_fp128)
-; N32-DAG:        ld  [[R2:\$[0-9]+]], %lo(struct_fp128)([[R1]])
-; N32-DAG:        dmtc1 [[R2]], $f0
+; N32-DAG:        ldc1 $f0, %lo(struct_fp128)([[R1]])
 ; N32-DAG:        addiu [[R3:\$[0-9]+]], [[R1]], %lo(struct_fp128)
-; N32-DAG:        ld  [[R4:\$[0-9]+]], 8([[R3]])
-; N32-DAG:        dmtc1 [[R4]], $f1
+; N32-DAG:        ldc1  $f1, 8([[R3]])
 
 ; N64-DAG:        lui  [[R1:\$[0-9]+]], %highest(struct_fp128)
-; N64-DAG:        ld  [[R2:\$[0-9]+]], %lo(struct_fp128)([[R1]])
-; N64-DAG:        dmtc1 [[R2]], $f0
-; N64-DAG:        ld  [[R4:\$[0-9]+]], 8([[R1]])
-; N64-DAG:        dmtc1 [[R4]], $f1
+; N64-DAG:        ldc1 $f0, %lo(struct_fp128)([[R1]])
+; N64-DAG:        ldc1 $f1, 8([[R1]])

test/CodeGen/Mips/msa/bitcast.ll

entry:
   %0 = load volatile <8 x half>, <8 x half>* %src
   %1 = bitcast <8 x half> %0 to <16 x i8>
   %2 = tail call <16 x i8> @llvm.mips.addv.b(<16 x i8> %1, <16 x i8> %1)
   store <16 x i8> %2, <16 x i8>* %dst
   ret void
 }
 
 ; LITENDIAN: v8f16_to_v16i8:
-; LITENDIAN: ld.h [[R1:\$w[0-9]+]],
+; LITENDIAN: ld.b [[R1:\$w[0-9]+]],
 ; LITENDIAN: addv.b [[R3:\$w[0-9]+]], [[R1]], [[R1]]
 ; LITENDIAN: st.b [[R3]],
 ; LITENDIAN: .size v8f16_to_v16i8
 
 ; BIGENDIAN: v8f16_to_v16i8:
-; BIGENDIAN: ld.h [[R1:\$w[0-9]+]],
-; BIGENDIAN: shf.b [[R3:\$w[0-9]+]], [[R1]], 177
+; BIGENDIAN: ld.b [[R1:\$w[0-9]+]],
 ; BIGENDIAN: addv.b [[R4:\$w[0-9]+]], [[R2]], [[R2]]
 ; BIGENDIAN: st.b [[R4]],
 ; BIGENDIAN: .size v8f16_to_v16i8
 
 ; We can't prevent the (bitcast (load X)) DAG Combine here because there
 ; are no operations for v8f16 to put in the way.
 define void @v8f16_to_v8i16(<8 x half>* %src, <8 x i16>* %dst) nounwind {
 entry:
entry:
   %0 = load volatile <8 x half>, <8 x half>* %src
   %1 = bitcast <8 x half> %0 to <4 x i32>
   %2 = tail call <4 x i32> @llvm.mips.addv.w(<4 x i32> %1, <4 x i32> %1)
   store <4 x i32> %2, <4 x i32>* %dst
   ret void
 }
 
 ; LITENDIAN: v8f16_to_v4i32:
-; LITENDIAN: ld.h [[R1:\$w[0-9]+]],
+; LITENDIAN: ld.w [[R1:\$w[0-9]+]],
 ; LITENDIAN: addv.w [[R2:\$w[0-9]+]], [[R1]], [[R1]]
 ; LITENDIAN: st.w [[R2]],
 ; LITENDIAN: .size v8f16_to_v4i32
 
 ; BIGENDIAN: v8f16_to_v4i32:
-; BIGENDIAN: ld.h [[R1:\$w[0-9]+]],
-; BIGENDIAN: shf.h [[R2:\$w[0-9]+]], [[R1]], 177
+; BIGENDIAN: ld.w [[R1:\$w[0-9]+]],
 ; BIGENDIAN: addv.w [[R3:\$w[0-9]+]], [[R2]], [[R2]]
 ; BIGENDIAN: st.w [[R3]],
 ; BIGENDIAN: .size v8f16_to_v4i32
 
 ; We can't prevent the (bitcast (load X)) DAG Combine here because there
 ; are no operations for v8f16 to put in the way.
 define void @v8f16_to_v4f32(<8 x half>* %src, <4 x float>* %dst) nounwind {
 entry:
   %0 = load volatile <8 x half>, <8 x half>* %src
   %1 = bitcast <8 x half> %0 to <4 x float>
   %2 = tail call <4 x float> @llvm.mips.fadd.w(<4 x float> %1, <4 x float> %1)
   store <4 x float> %2, <4 x float>* %dst
   ret void
 }
 
 ; LITENDIAN: v8f16_to_v4f32:
-; LITENDIAN: ld.h [[R1:\$w[0-9]+]],
+; LITENDIAN: ld.w [[R1:\$w[0-9]+]],
 ; LITENDIAN: fadd.w [[R2:\$w[0-9]+]], [[R1]], [[R1]]
 ; LITENDIAN: st.w [[R2]],
 ; LITENDIAN: .size v8f16_to_v4f32
 
 ; BIGENDIAN: v8f16_to_v4f32:
-; BIGENDIAN: ld.h [[R1:\$w[0-9]+]],
-; BIGENDIAN: shf.h [[R2:\$w[0-9]+]], [[R1]], 177
+; BIGENDIAN: ld.w [[R1:\$w[0-9]+]],
 ; BIGENDIAN: fadd.w [[R3:\$w[0-9]+]], [[R2]], [[R2]]
 ; BIGENDIAN: st.w [[R3]],
 ; BIGENDIAN: .size v8f16_to_v4f32
 
 ; We can't prevent the (bitcast (load X)) DAG Combine here because there
 ; are no operations for v8f16 to put in the way.
 define void @v8f16_to_v2i64(<8 x half>* %src, <2 x i64>* %dst) nounwind {
 entry:
   %0 = load volatile <8 x half>, <8 x half>* %src
   %1 = bitcast <8 x half> %0 to <2 x i64>
   %2 = tail call <2 x i64> @llvm.mips.addv.d(<2 x i64> %1, <2 x i64> %1)
   store <2 x i64> %2, <2 x i64>* %dst
   ret void
 }
 
 ; LITENDIAN: v8f16_to_v2i64:
-; LITENDIAN: ld.h [[R1:\$w[0-9]+]],
+; LITENDIAN: ld.d [[R1:\$w[0-9]+]],
 ; LITENDIAN: addv.d [[R2:\$w[0-9]+]], [[R1]], [[R1]]
 ; LITENDIAN: st.d [[R2]],
 ; LITENDIAN: .size v8f16_to_v2i64
 
 ; BIGENDIAN: v8f16_to_v2i64:
-; BIGENDIAN: ld.h [[R1:\$w[0-9]+]],
-; BIGENDIAN: shf.h [[R2:\$w[0-9]+]], [[R1]], 27
+; BIGENDIAN: ld.d [[R1:\$w[0-9]+]],
 ; BIGENDIAN: addv.d [[R3:\$w[0-9]+]], [[R2]], [[R2]]
 ; BIGENDIAN: st.d [[R3]],
 ; BIGENDIAN: .size v8f16_to_v2i64
 
 ; We can't prevent the (bitcast (load X)) DAG Combine here because there
 ; are no operations for v8f16 to put in the way.
 define void @v8f16_to_v2f64(<8 x half>* %src, <2 x double>* %dst) nounwind {
 entry:
   %0 = load volatile <8 x half>, <8 x half>* %src
   %1 = bitcast <8 x half> %0 to <2 x double>
   %2 = tail call <2 x double> @llvm.mips.fadd.d(<2 x double> %1, <2 x double> %1)
   store <2 x double> %2, <2 x double>* %dst
   ret void
 }
 
 ; LITENDIAN: v8f16_to_v2f64:
-; LITENDIAN: ld.h [[R1:\$w[0-9]+]],
+; LITENDIAN: ld.d [[R1:\$w[0-9]+]],
 ; LITENDIAN: fadd.d [[R2:\$w[0-9]+]], [[R1]], [[R1]]
 ; LITENDIAN: st.d [[R2]],
 ; LITENDIAN: .size v8f16_to_v2f64
 
 ; BIGENDIAN: v8f16_to_v2f64:
-; BIGENDIAN: ld.h [[R1:\$w[0-9]+]],
-; BIGENDIAN: shf.h [[R2:\$w[0-9]+]], [[R1]], 27
+; BIGENDIAN: ld.d [[R1:\$w[0-9]+]],
 ; BIGENDIAN: fadd.d [[R3:\$w[0-9]+]], [[R2]], [[R2]]
 ; BIGENDIAN: st.d [[R3]],
 ; BIGENDIAN: .size v8f16_to_v2f64
 ;----
 
 define void @v4i32_to_v16i8(<4 x i32>* %src, <16 x i8>* %dst) nounwind {
 entry:
   %0 = load volatile <4 x i32>, <4 x i32>* %src