[x86] When using "and $0" and "orl $-1" to store 0 and -1 for minsize, make sure the store isn't volatile

[craig.topper]

Summary

If the store is volatile this might be a memory mapped IO access. In that case we shouldn't generate a load that didn't exist in the source

[RKSimon]

lib/Target/X86/X86InstrCompiler.td

1046 ↗

(On Diff #159088)

Is there any place more generic we can put this? It doesn't seem x86 specific

[craig.topper]

lib/Target/X86/X86InstrCompiler.td

1046 ↗

(On Diff #159088)

It should probably be hoisted to include/llvm/Target/TargetSelectionDAG.td. SystemZ has this already with this name so we'd have to change that too. Do you want me move it and change SystemZ as a pre-patch. Or do you want it folded in here? Or move SystemZ as a followup?

[RKSimon]

LGTM - probably worth committing the new tests with current codegen as a pre-commit.

lib/Target/X86/X86InstrCompiler.td

1046 ↗

(On Diff #159088)

Leaving it here and doing the X86/SystemZ merge into TargetSelectionDAG.td as a followup is fine by me.

[hans]

Thanks for fixing this, Craig!

llvm/trunk/lib/Target/X86/X86InstrCompiler.td

 def : Pat<(i16 (atomic_load_16 addr:$src)), (MOV16rm addr:$src)>;
 def : Pat<(i32 (atomic_load_32 addr:$src)), (MOV32rm addr:$src)>;
 def : Pat<(i64 (atomic_load_64 addr:$src)), (MOV64rm addr:$src)>;
 
 //===----------------------------------------------------------------------===//
 // DAG Pattern Matching Rules
 //===----------------------------------------------------------------------===//
 
+def nonvolatile_store : PatFrag<(ops node:$val, node:$ptr),
+                                 (store node:$val, node:$ptr), [{
+  return !cast<StoreSDNode>(N)->isVolatile();
+}]>;
+
+
 // Use AND/OR to store 0/-1 in memory when optimizing for minsize. This saves
 // binary size compared to a regular MOV, but it introduces an unnecessary
 // load, so is not suitable for regular or optsize functions.
 let Predicates = [OptForMinSize] in {
-def : Pat<(store (i16 0), addr:$dst), (AND16mi8 addr:$dst, 0)>;
-def : Pat<(store (i32 0), addr:$dst), (AND32mi8 addr:$dst, 0)>;
-def : Pat<(store (i64 0), addr:$dst), (AND64mi8 addr:$dst, 0)>;
-def : Pat<(store (i16 -1), addr:$dst), (OR16mi8 addr:$dst, -1)>;
-def : Pat<(store (i32 -1), addr:$dst), (OR32mi8 addr:$dst, -1)>;
-def : Pat<(store (i64 -1), addr:$dst), (OR64mi8 addr:$dst, -1)>;
+def : Pat<(nonvolatile_store (i16 0), addr:$dst), (AND16mi8 addr:$dst, 0)>;
+def : Pat<(nonvolatile_store (i32 0), addr:$dst), (AND32mi8 addr:$dst, 0)>;
+def : Pat<(nonvolatile_store (i64 0), addr:$dst), (AND64mi8 addr:$dst, 0)>;
+def : Pat<(nonvolatile_store (i16 -1), addr:$dst), (OR16mi8 addr:$dst, -1)>;
+def : Pat<(nonvolatile_store (i32 -1), addr:$dst), (OR32mi8 addr:$dst, -1)>;
+def : Pat<(nonvolatile_store (i64 -1), addr:$dst), (OR64mi8 addr:$dst, -1)>;
 }
 
 // In kernel code model, we can get the address of a label
 // into a register with 'movq'.  FIXME: This is a hack, the 'imm' predicate of
 // the MOV64ri32 should accept these.
 def : Pat<(i64 (X86Wrapper tconstpool  :$dst)),
           (MOV64ri32 tconstpool  :$dst)>, Requires<[KernelCode]>;
 def : Pat<(i64 (X86Wrapper tjumptable  :$dst)),

llvm/trunk/test/CodeGen/X86/store-zero-and-minus-one.ll

 
 }
 
 ; FIXME: Make sure we don't use the and/or trick on volatile stores.
 define void @volatile_zero_64(i64* %p) minsize {
 ; CHECK32-LABEL: volatile_zero_64:
 ; CHECK32:       # %bb.0: # %entry
 ; CHECK32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK32-NEXT:    andl $0, 4(%eax)
-; CHECK32-NEXT:    andl $0, (%eax)
+; CHECK32-NEXT:    xorl %ecx, %ecx
+; CHECK32-NEXT:    movl %ecx, 4(%eax)
+; CHECK32-NEXT:    movl %ecx, (%eax)
 ; CHECK32-NEXT:    retl
 ;
 ; CHECK64-LABEL: volatile_zero_64:
 ; CHECK64:       # %bb.0: # %entry
-; CHECK64-NEXT:    andq $0, (%rdi)
+; CHECK64-NEXT:    movq $0, (%rdi)
 ; CHECK64-NEXT:    retq
 entry:
   store volatile i64 0, i64* %p
   ret void
 }
 
 define void @volatile_zero_32(i32* %p) minsize {
 ; CHECK32-LABEL: volatile_zero_32:
 ; CHECK32:       # %bb.0: # %entry
 ; CHECK32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK32-NEXT:    andl $0, (%eax)
+; CHECK32-NEXT:    movl $0, (%eax)
 ; CHECK32-NEXT:    retl
 ;
 ; CHECK64-LABEL: volatile_zero_32:
 ; CHECK64:       # %bb.0: # %entry
-; CHECK64-NEXT:    andl $0, (%rdi)
+; CHECK64-NEXT:    movl $0, (%rdi)
 ; CHECK64-NEXT:    retq
 entry:
   store volatile i32 0, i32* %p
   ret void
 }
 
 define void @volatile_zero_16(i16* %p) minsize {
 ; CHECK32-LABEL: volatile_zero_16:
 ; CHECK32:       # %bb.0: # %entry
 ; CHECK32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK32-NEXT:    andw $0, (%eax)
+; CHECK32-NEXT:    movw $0, (%eax)
 ; CHECK32-NEXT:    retl
 ;
 ; CHECK64-LABEL: volatile_zero_16:
 ; CHECK64:       # %bb.0: # %entry
-; CHECK64-NEXT:    andw $0, (%rdi)
+; CHECK64-NEXT:    movw $0, (%rdi)
 ; CHECK64-NEXT:    retq
 entry:
   store volatile i16 0, i16* %p
   ret void
 }
 
 
 define void @volatile_minus_one_64(i64* %p) minsize {
 ; CHECK32-LABEL: volatile_minus_one_64:
 ; CHECK32:       # %bb.0: # %entry
 ; CHECK32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK32-NEXT:    orl $-1, 4(%eax)
-; CHECK32-NEXT:    orl $-1, (%eax)
+; CHECK32-NEXT:    xorl %ecx, %ecx
+; CHECK32-NEXT:    decl %ecx
+; CHECK32-NEXT:    movl %ecx, 4(%eax)
+; CHECK32-NEXT:    movl %ecx, (%eax)
 ; CHECK32-NEXT:    retl
 ;
 ; CHECK64-LABEL: volatile_minus_one_64:
 ; CHECK64:       # %bb.0: # %entry
-; CHECK64-NEXT:    orq $-1, (%rdi)
+; CHECK64-NEXT:    movq $-1, (%rdi)
 ; CHECK64-NEXT:    retq
 entry:
   store volatile i64 -1, i64* %p
   ret void
 }
 
 define void @volatile_minus_one_32(i32* %p) minsize {
 ; CHECK32-LABEL: volatile_minus_one_32:
 ; CHECK32:       # %bb.0: # %entry
 ; CHECK32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK32-NEXT:    orl $-1, (%eax)
+; CHECK32-NEXT:    movl $-1, (%eax)
 ; CHECK32-NEXT:    retl
 ;
 ; CHECK64-LABEL: volatile_minus_one_32:
 ; CHECK64:       # %bb.0: # %entry
-; CHECK64-NEXT:    orl $-1, (%rdi)
+; CHECK64-NEXT:    movl $-1, (%rdi)
 ; CHECK64-NEXT:    retq
 entry:
   store volatile i32 -1, i32* %p
   ret void
 }
 
 define void @volatile_minus_one_16(i16* %p) minsize {
 ; CHECK32-LABEL: volatile_minus_one_16:
 ; CHECK32:       # %bb.0: # %entry
 ; CHECK32-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; CHECK32-NEXT:    orw $-1, (%eax)
+; CHECK32-NEXT:    movw $-1, (%eax)
 ; CHECK32-NEXT:    retl
 ;
 ; CHECK64-LABEL: volatile_minus_one_16:
 ; CHECK64:       # %bb.0: # %entry
-; CHECK64-NEXT:    orw $-1, (%rdi)
+; CHECK64-NEXT:    movw $-1, (%rdi)
 ; CHECK64-NEXT:    retq
 entry:
   store volatile i16 -1, i16* %p
   ret void
 }

llvm/trunk/test/CodeGen/X86/tail-opts.ll

return:
   ret void
 }
 
 ; two_minsize - Same as two, but with minsize instead of optsize.
 
 ; CHECK-LABEL: two_minsize:
 ; CHECK-NOT: XYZ
 ; CHECK: ret
-; CHECK: andl $0, XYZ(%rip)
+; CHECK: movl $0, XYZ(%rip)
 ; CHECK: movl $1, XYZ(%rip)
 ; CHECK-NOT: XYZ
 
 define void @two_minsize() nounwind minsize {
 entry:
   %0 = icmp eq i32 undef, 0
   br i1 %0, label %bbx, label %bby