[PATCH] D28540: [NVPTX] Added support for half-precision floating point.

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp:754
   else if (ScalarVT.isFloatingPoint())
-    fromType = NVPTX::PTXLdStInstCode::Float;
+    // f16 uses untyped integer load, as there's no ld.f16 instruction.
+    fromType = ScalarVT.SimpleTy == MVT::f16 ? NVPTX::PTXLdStInstCode::Untyped
----------------
b16


================
Comment at: lib/Target/NVPTX/NVPTXInstrInfo.td:156
 
+def hasFP16Math: Predicate<"Subtarget->allowFP16Math()">;
 
----------------
Can we match the predicate name to the name of the subtarget function?  Unless you think that would be more confusing than not.


================
Comment at: lib/Target/NVPTX/NVPTXSubtarget.h:104
   bool hasImageHandles() const;
+  bool hasFP16() const { return SmVersion >= 53; }
+  bool allowFP16Math() const;
----------------
jlebar wrote:
> tra wrote:
> > jlebar wrote:
> > > Do we need this function at all?  It's kind of confusing because pre-sm_53 we had fp16 loads/stores, so you could say we "had" fp16.  But also someone might accidentally call this instead of allowFP16Math().
> > Renamed to hasFP16Math.
> Yeah, but still...do we need it?  It's confusing that the predicates in the .td are called "hasFP16Math" but they actually correspond to allowFP16Math, and I'm afraid in general someone will call hasFP16Math when they mean to call allowFP16Math.  It seems that nobody needs this function today, so can we remove it and inline the SM version check into allowFP16Math?
> 
> We might also want to change the tablegen predicate to match this function name.
^ This is my only remaining nontrivial concern about the patch.  Otherwise, looks great to me.


https://reviews.llvm.org/D28540