[PATCH] D55897: Add constrained fptrunc and fpext intrinsics

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:2787
+  case ISD::STRICT_FP_ROUND:
+    Tmp1 = EmitStackConvert(Node->getOperand(1), 
+                            Node->getValueType(0),
----------------
kpn wrote:
> andrew.w.kaylor wrote:
> > Can this cause the wrong rounding mode to be used? For X86 targets I would guess it will result in an instruction that uses the runtime rounding mode, but I'm not sure we can count on that for all targets. Also, if the value being converted is a constant, might this get folded using the default rounding mode? And if it is a constant and we knew the rounding mode based on an argument to the intrinsic, we might want to fold it (though ideally that would have happened before this).
> Isn't this an argument for a strict load node types and maybe a strict store type as well? 
> 
> I'd be surprised if we were doing constant folding of a store+load combination, but if we are then either a strict load or store node would disable that folding.
> 
> Strictly speaking, using the runtime rounding mode would be incorrect if that wasn't what was specified in the rounding mode field of the intrinsic. So even X86 could be wrong.
> 
> We aren't doing anything with the rounding and exception arguments yet for any of the new intrinsics. So how about I file a bug noting that fact and noting that we need to not forget about this case? Then we can move on and come back later.
Actually, if the runtime rounding mode doesn't match the rounding argument for non-dynamic cases that's a user error or a bug somewhere upstream in the compiler. The rounding mode argument is supposed to tell us what the rounding mode is at this point in the program. It is not supposed to control the rounding mode. The non-dynamic rounding modes are essentially an "assume" kind of directive.

I see that the constant folding case I'm concerned about will actually be covered by one of your test cases, so I think we can safely move forward with this the way you have it.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1651
   // Otherwise split it by hand.
-  EVT InVT = N->getOperand(0).getValueType();
+  unsigned OpNo = N->isStrictFPOpcode();
+  EVT InVT = N->getOperand(OpNo).getValueType();
----------------
I'd prefer to see this as:

unsigned OpNo = N->isStrictFPOpcode() ? 1 : 0;



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2043
   SDLoc dl(N);
-  GetSplitVector(N->getOperand(0), Lo, Hi);
+  GetSplitVector(N->getOperand(N->isStrictFPOpcode()), Lo, Hi);
   EVT InVT = Lo.getValueType();
----------------
Again, the use of a boolean result as an index is awkward.


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:303
+
+; Verify that fpext(42.1) isn't simplified when the rounding mode is
+; unknown.
----------------
This comment is wrong.


================
Comment at: test/CodeGen/X86/vector-constrained-fp-intrinsics.ll:3
 ; RUN: llc -O3 -mtriple=x86_64-pc-linux < %s | FileCheck %s
+; RUN: llc -O3 -mtriple=x86_64-pc-linux -mattr=+avx < %s | FileCheck --check-prefix=AVX %s
 
----------------
craig.topper wrote:
> Do this as a pre-commit?
Expanding on Craig's comment here. I think he is suggesting that you add the AVX run line and all of the associated new checks as a separate patch before the fpext/fptrrunc patch lands.


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

https://reviews.llvm.org/D55897