[PATCH] D143601: Add LLVM type support for fp8

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

I'm not sure if the patch is ready for review, so just a drive-by comments. Please add me as reviewer once you're ready and I'll take a closer look.

Overall looks OK with two caveats:

- predicates need to be thoroughly checked. AFAICT all FP8 types need PTX 7.8  and SM_90, but the patch seems to ahve copy/pasted PTX7.0/SM_80 in many places.
- the patch needs a lot more tests.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:186
      !eq(name, "i64"): Int64Regs,
+     !eq(name, "f8e4m3"): Int1Regs,
      !eq(name, "f16"): Float16Regs,
----------------
Typo? `Int16Regs`?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:540
+                !strconcat("cvt${mode:base}${mode:ftz}${mode:sat}.",
+                FromName, ".f8e4m3 \t$dst, $src;"), []>;
     def _f16 :
----------------
Shouldn't these be predicated on ptx7.8/sm_90?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:592
+                FromName, ".f16 \t$dst, $src;"), []>,
+                Requires<[hasPTX70, hasSM90]>;
+  }
----------------
I think all conversion instructions fo/tfom FP8 were introduced in PTX7.8.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:621
+                FromName, ".f16x2 \t$dst, $src1, $src2;"), []>,
+    Requires<[hasPTX70, hasSM80]>;
+  }
----------------
Nit: indentation should match that of `NVPTXInst` above.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:626
+
+  defm CVT_f8e4m3 : CVT_FROM_FP8_SM80<"f8e4m3", Int1Regs>;
+  
----------------
Nit. I'd drop `_SM80`  from the names.  


================
Comment at: llvm/lib/Target/NVPTX/NVPTXInstrInfo.td:2466
+  def ProxyRegI64      : ProxyRegInst<"b64",  i64, Int64Regs>;
+  //def ProxyRegF8e5m2   : ProxyRegInst<"b8",   f8e5m2, Float8e5m2Regs>;
+  def ProxyRegF16      : ProxyRegInst<"b16",  f16, Float16Regs>;
----------------
What's the story here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143601