[PATCH] D99675: [llvm][clang] Create new intrinsic llvm.arith.fence to control FP optimization at expression level

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:903
+                                  const Twine &Name = "") {
+    return CreateIntrinsic(Intrinsic::arithmetic_fence, {DstType}, {Val}, nullptr,
+                           Name);
----------------
Do you really need curly braces around DstType and Val? A single value should be implicitly convertible to ArrayRef.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1333
+
+ // Intrinsics to support half precision floating point format
 // Intrinsics to support half precision floating point format
----------------
This comment got duplicated.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1332
+      case TargetOpcode::ARITH_FENCE:
+        OutStreamer->emitRawComment("ARITH_FENCE");
+        break;
----------------
I think you should check isVerbose() before printing this.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3149
   case ISD::FREEZE:
+  case ISD::ARITH_FENCE:
   case ISD::FCANONICALIZE:
----------------
What about splitting a vector like v8f32 on SSE2?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6296
+  case Intrinsic::arithmetic_fence: {
+    auto DL = getCurSDLoc();
+
----------------
There's already a variable called sdl that contains this. It's used in the surrounding cases.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6299
+    SDValue Val = getValue(I.getArgOperand(0));
+    EVT ResultVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
+
----------------
Why isn't this just Val.getValueType()?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99675/new/

https://reviews.llvm.org/D99675