[PATCH] D149715: IR: Add llvm.frexp intrinsic

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/LangRef.rst:14767
+This intrinsic splits a floating point value into a normalized
+fractional component and integral exponent.
+
----------------
sepavloff wrote:
> integral -> integer?
No, every other doc phrases it as integral 


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:1831
 
+  /// Build and insert \p Fract, \p Exp = G_FLDEXP \p Src
+  MachineInstrBuilder
----------------
foad wrote:
> sepavloff wrote:
> > G_FLDEXP -> G_FFREXP?
> "G_FFREXP"
I'm debating doing a cleanup and dropping all of the "F" prefixes from all of these. It's copying an annoyance from the DAG where the node names don't exactly match the IR 


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1043
+  // TODO: Should constrain all element counts to match
+  def int_frexp : DefaultAttrsIntrinsic<[llvm_anyfloat_ty, llvm_anyint_ty], [LLVMMatchType<0>]>;
 }
----------------
sepavloff wrote:
> Can `LLVMScalarOrSameVectorWidth` be used to constrain return values?
I tried to use it a few times, but it seems to not work. I think it's having trouble with the type being embedded inside this argument list.


```
/Users/matt/src/llvm-project/llvm/include/llvm/IR/Intrinsics.td:1046:7: error: Initializer of 'TypeSig' in 'int_frexp' could not be fully resolved: anonymous_142.TypeSig
  def int_frexp : DefaultAttrsIntrinsic<[llvm_anyfloat_ty, LLVMScalarOrSameVectorWidth<0, llvm_anyint_ty>], [LLVMMatchType<0>]>;
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2086
+    Entry.IsSExt = TLI.shouldSignExtendTypeInLibCall(ArgVT, isSigned);
+    Entry.IsZExt = !TLI.shouldSignExtendTypeInLibCall(ArgVT, isSigned);
+    Args.push_back(Entry);
----------------
sepavloff wrote:
> Can it be just `Entry.IsZExt = !Entry.IsSExt`?
This is just copy paste from the other case that does this. Seems like a broken API though, why split out the 2 cases if it must be one or the other?


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

https://reviews.llvm.org/D149715