[PATCH] D36335: Add ‘llvm.experimental.constrained.fma‘ Intrinsic

Andy Kaylor via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 14 10:22:11 PDT 2017


andrew.w.kaylor added inline comments.


================
Comment at: docs/LangRef.rst:13035
+The fourth and fifth arguments specify the rounding mode and exception
+behavior as described above. Note that the rounding happens only once here.
+
----------------
I'm not sure it's clear what the comment "Note that the rounding happens only once here" means in this context.  The rounding mode argument provides information to the optimizer and does not have any functional effect.  I hope that this is straightforward enough with the other intrinsics that the terse comments there were sufficient.

In the case of the constrained fma intrinsic, it is worth mentioning that any actions the optimizer performs on the intrinsic must be consistent with the rounding behavior of an fma instruction.  For instance, the optimizer cannot perform constant folding where a rounded multiply is performed followed a rounded add -- the rounding must be atomic.  Perhaps that is what you intended to say here.  If so, I believe a more verbose statement is needed.


================
Comment at: docs/LangRef.rst:13037
+
+Semantics:
+""""""""""
----------------
I think it would be a good idea to discuss here the circumstances under which this intrinsic can be formed.  Specifically, what is the relationship between rounding mode control and the fp-contract setting.  If strict rounding behavior is required within a scope, but fusing is enabled globally within the compilation unit does the rounding requirement override the fp-contract setting?  I think it should.

Also, what are the expected exception semantics?  If a scope is governed by strict exception behavior, how will the FP status flags be handled if a multiply and an add are fused?  I believe what is required is that if either operation would have set an FP status flag then the fused operation must also set that flag, and no flag should be set by the fused operation that would not have been set by either of the two operations separately.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2015
   default: break;
+  case ISD::FMA: {
+    SDValue ISDFMA = CurDAG->getNode(X86ISD::FMADD, SDLoc(Node),
----------------
Can you explain why this was necessary?  I would have expected there to have been handling already in place for ISD::FMA.


Repository:
  rL LLVM

https://reviews.llvm.org/D36335





More information about the llvm-commits mailing list