[PATCH] D69562: Mapping of FP operations to constrained intrinsics

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 4 inline comments as done.
sepavloff added inline comments.


================
Comment at: llvm/lib/IR/FloatingPoint.cpp:85
+  case Instruction::FAdd:
+    IID = Intrinsic::experimental_constrained_fadd;
+    break;
----------------
arsenm wrote:
> I would prefer direct returns rather than variable assign and break
Single exit point has advantages. For instance we could set a watch in debugger on return statement and filter interesting cases.


================
Comment at: llvm/lib/IR/FloatingPoint.cpp:172
+      default:
+        break;
+      }
----------------
kpn wrote:
> arsenm wrote:
> > arsenm wrote:
> > > I think this should assert or something on unhandled target intrinsics. Right now it will just return then original ID which is probably not expected
> > Nevermind, I can't read. It will return not_intrinsic although I still think this is a possible source of error
> Honestly, I agree that this being a possible source of error. But I'm not sure we have an easy check that callers can make to find out if they should be calling this function in the first place. Without an easy check we're left with calling this function and checking the return value.
This function must work correctly if `Instr` does not represent floating point operation. This property is mentioned in the function documentation. It is used in Inliner (D69798) to check if current instruction requires replacement with constrained intrinsic or may be processed with `clone` method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69562