Guard FMF context by excluding some FP operators from FPMathOperator

[mcberg2017]

Summary

Some FPMathOperators succeed and the retrieve FMF context when they never have it, we should omit these cases to keep from removing FMF context.

For instance when we visit some FPMathOperator mapped Instructions which never have FMF flags and a Node was associated which does have FMF flags, that Node today will have all its flags cleared via the intersect operation. With this change, we exclude associating Nodes that never have FPMathOperator status under FMF.

[mcberg2017]

The attached test is a x86 approximation of internal tests, altered for platform, which show this problem which occurs frequently on many FMF optimization contexts.

[mcberg2017]

Comments, alternatives?

[spatel]

Adding Amara as reviewer. The AnyDefined flag bit was added with:
D32527

It's not clear to me how copying flags to a node does not define them. It might help if you explain what is happening in the test. Also, is that the minimal test? I don't understand why we need vector ops and type conversions. It would be better to show the complete asm output using utils/update_llc_test_checks.py, and this patch should show the diff from the output of trunk.

[mcberg2017]

Copying all available flags as false should not change the initial state of the object after its creation. It only does so because we are using AnyDefined as a sticky bit for assignment of any flag regardless of value. My notion is that any true value indicates that at least a value is set to something other than a default value (false). The test conditions occurs in partial vectorization and in our case, after scalarization on gpus where interior math operations occur. It happens frequently in our cases. The problem is when an instruction is associated to a Node that does have flags and where the Instruction it is associated to does not. This only ever occurs where the association is disjunct, meaning the Node and the Instruction originated differently. Generally its the other way around, where Node is empty and we are inheriting flags from an Instruction as the Instruction originated the Node in question. All other operations of intersect are done from Node to Node, ergo this change does not modify that behavior, that only occurs in this context.

[mcberg2017]

I will have both with and without assembler example shortly, added here. As for the test case, it is pretty close to minimal with context needed for association, but I will see if I can shrink it a bit after the assembler is up.

[mcberg2017]

without the change:

pushl %eax
.cfi_def_cfa_offset 8
vmovss 12(%esp), %xmm0 # xmm0 = mem[0],zero,zero,zero
vmulss 8(%esp), %xmm0, %xmm1
vaddss %xmm0, %xmm1, %xmm0
vmovss %xmm0, (%esp)
flds (%esp)
popl %eax
.cfi_def_cfa_offset 4
retl

with the change:

pushl %eax
.cfi_def_cfa_offset 8
vmovss 12(%esp), %xmm0 # xmm0 = mem[0],zero,zero,zero
vfmadd132ss 8(%esp), %xmm0, %xmm0 # xmm0 = (xmm0 * mem) + xmm0
vmovss %xmm0, (%esp)
flds (%esp)
popl %eax
.cfi_def_cfa_offset 4
retl

[mcberg2017]

I have a smaller test case now, its baseline analog should show up in the tree in a bit...

[mcberg2017]

With the test file sync'd

[aemerson]

So with this change, if you have:

StrictInst = an FP instruction with no flags.
FastInst = an FP instruction with a flag.
SDNode A;
A.copyFMF(StrictInst);
SDNode B;
B.copyFMF(FastInst);
B.intersectWith(A);

[aemerson]

Hit submit too early:
So with the above code B will not have the flag cleared? If that's the case it doesn't seem right to me.

[mcberg2017]

Correctly stated, but out of context. Some clarity first. The kinds of instructions in our case are neither strict nor fast, they never carry flags. Neither scalarization nor partial vectorization should cause FMF obliteration. Perhaps we need additional mechanisms?

[mcberg2017]

Another data point, this change caused no drift in the lit tests, meaning none of our FMF opts managed in the lit tests are modified by the behavior.

[aemerson]

I’m not seeing how FP instructions can not carry flags. The IR semantics are defined to be strict FP unless explicitly relaxed.

The point of the AnyDefined state bit is to avoid pessimizing when the actual flags object has not been explicitly written to, e.g default constructed. When instantiating from a math operator that must obey strict semantics by default, the flags must also reflect those same semantics. Now if FPMathOperator also has a 3rd “undefined state” then it’s different.

[mcberg2017]

In the newly created test case above, as checked in for test/CodeGen/X86/intersect-fma-fmf.ll, we get precisely this scenario. From a debug session breakpointed in llvm::SelectionDAGBuilder::visit at the line containing Node->intersectFlagsWith(IncomingFlags);
The Instruction in question is: %tmp11 = extractelement <4 x float> %tmp10, i32 1 as seen by the debugger, the Node we are associating it to is: t7: f32 = fadd nnan ninf nsz arcp contract afn reassoc t6, t5.
The extractelement is an Instruction that never carries flags and qualifies under

if (auto *FPMO = dyn_cast<FPMathOperator>(&I))

as a FPMathOperator, so it looks like we have the 3rd type. SelectionDAGBuilder::visitExtractElement results in the association under setValue.

[aemerson]

Ok, to me it seems like instructions for which fast math flags can't apply shouldn't be classed as FPMathOperators. insertelement/extractelement are just moving raw bits. If we disallowed those instruction types in the FPMathOperator classof() then this problem should go away, and we don't incorrectly relax FP semantics like in the case I pointed out.

[mcberg2017]

Thanks, that is a good alternative. I am testing it internally to see I get similar coverage. More later...

[mcberg2017]

I added one more non fmf instruction and there is room to add others if needed.

[spatel]

In D51145#1223770, @mcberg2017 wrote:

I added one more non fmf instruction and there is room to add others if needed.

There's also discussion about the definition of FPMathOperator and the relation to FMF here:
https://bugs.llvm.org/show_bug.cgi?id=38086
D51646

[aemerson]

In D51145#1224873, @spatel wrote:

In D51145#1223770, @mcberg2017 wrote:

I added one more non fmf instruction and there is room to add others if needed.

There's also discussion about the definition of FPMathOperator and the relation to FMF here:
https://bugs.llvm.org/show_bug.cgi?id=38086
D51646

Thanks.

I think this approach makes sense, given that the langref only defines a restricted few instructions to carry fast-math flags, not the entire group that the current FPMathOperator allows. But I'd like someone else to check it over. @hfinkel does this look reasonable?

[mcberg2017]

Hal/Sanjay could one of you have a look at this code, it would be great to wrap it up...

[spatel]

include/llvm/IR/Operator.h

394–395

I don't see the benefit of treating Instruction/ConstantExpr separately when the logic is identical for both and likely to keep growing.

Ie, can we do this:

static bool classof(const Value *V) {
  unsigned Opcode;
  if (auto *I = dyn_cast<Instruction>(V))
    Opcode = I->getOpcode();
  else if (auto *CE = dyn_cast<ConstantExpr>(V))
    Opcode = CE->getOpcode();
  else
    return false;

  switch (Opcode) {
  case Instruction::FCmp:
    return true;
  // Vector operators may have FP type, but they don't do math.
  case Instruction::ExtractElement:
  case Instruction::ShuffleVector:
  case Instruction::InsertElement:
    return false;
  default:
    return V->getType()->isFPOrFPVectorTy();
  }
}

[mcberg2017]

Collapsed for single version...

[spatel]

LGTM - I do wonder if it would be better to white-list just the FP ops that we know can carry FMF though (ie, make this consistent with IR verification)...unless FPMathOperator is used for some other purposes.

include/llvm/IR/Operator.h

public:
   }
 
   /// Get the maximum error permitted by this operation in ULPs. An accuracy of
   /// 0.0 means that the operation should be performed with the default
   /// precision.
   float getFPAccuracy() const;
 
   static bool classof(const Instruction *I) {
-    return I->getType()->isFPOrFPVectorTy() ||
-      I->getOpcode() == Instruction::FCmp;
+    switch (I->getOpcode()) {
+    case Instruction::FCmp:
+      return true;
+    case Instruction::ExtractElement:
+    case Instruction::ShuffleVector:
+    case Instruction::InsertElement:
+      return false;
+    default:
+      return I->getType()->isFPOrFPVectorTy();
+    }
   }
 
   static bool classof(const ConstantExpr *CE) {
-    return CE->getType()->isFPOrFPVectorTy() ||
-           CE->getOpcode() == Instruction::FCmp;
+    switch (CE->getOpcode()) {
+    case Instruction::FCmp:
+      return true;
+    case Instruction::ExtractElement:
+    case Instruction::ShuffleVector:
+    case Instruction::InsertElement:
+      return false;
+    default:
+      return CE->getType()->isFPOrFPVectorTy();
+    }
   }
 
   static bool classof(const Value *V) {
     return (isa<Instruction>(V) && classof(cast<Instruction>(V))) ||
            (isa<ConstantExpr>(V) && classof(cast<ConstantExpr>(V)));
   }
 };
 
 /// A helper template for defining operators for individual opcodes.
 template<typename SuperClass, unsigned Opc>
 class ConcreteOperator : public SuperClass {
 public:
   static bool classof(const Instruction *I) {

test/CodeGen/X86/intersect-fma-fmf.ll

 ; RUN: llc -mtriple=i686-unknown-unknown -mattr=+avx512f < %s | FileCheck %s
 
 define float @test_x86_fma_intersection_fmf(float %a, float %b) {
 ; CHECK-LABEL: test_x86_fma_intersection_fmf:
 ; CHECK:      # %bb.0:
-; CHECK:        vmulss {{[0-9]+}}(%esp), %xmm0, %xmm1
-; CHECK-NEXT:   vaddss %xmm0, %xmm1, %xmm0
+; CHECK:        vfmadd132ss {{[0-9]+}}(%esp), %xmm0, %xmm0
 ; CHECK:        retl 
      %tmp8 = fmul fast float %a, %b
      %tmp9 = fadd fast float %tmp8, %b
      %tmp10 = insertelement <4 x float> undef, float  %tmp9, i32 1
      %tmp11 = extractelement <4 x float> %tmp10, i32 1
      ret float %tmp11
 }