[PATCH] D87232: [SVE][CodeGen] Lower floating point -> integer conversions

Thu Sep 10 13:39:55 PDT 2020

efriedma added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:84
+  FP_TO_UINT_MERGE_PASSTHRU,
+  FP_TO_SINT_MERGE_PASSTHRU,
   FSUB_PRED,
----------------
paulwalker-arm wrote:
> efriedma wrote:
> > Probably should be named FP_TO_UINT_SAT_MERGE_PASSTHRU or something like that, if you're going to use it to lower llvm.aarch64.sve.fcvtzu.  (See D54749)
> I had a feeling you might say this.  The problem is this is more than a rename because the SAT nodes take an additional parameter.  This is not really a hurdle but there's a question as to where you draw the line.  All the intrinsics expect to produce results that match the instructions on which they're based.  This would suggest we can never use names based on similar ISD nodes because the common nodes do not define all corner cases.  This seems like overkill considering the _PRED/_MERGE nodes are all under the AArch64ISD namespace, which to me suggests they follow AArch64 semantics.
I think if there's some significant functionality difference, we should call it out in the name somehow.  There's a very high likelihood that these nodes, or something like them, will eventually become target-independent.  And even before that, people will likely assume they're equivalent based on the name.   I don't want something to subtly break because someone didn't realize there was a difference.  (It's particularly terrible in cases like fp->int conversions and shifts: if you confuse them, the result appears to mostly work, but you'll get a bug report years later.)

If you're not comfortable naming FP_TO_UINT_SAT_MERGE_PASSTHRU without the matching width parameter, we could just call it FCVTZU_MERGE_PASSTHRU .

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

https://reviews.llvm.org/D87232