[PATCH] D122918: [RISCV][CodeGen] Support Zfinx, Zdinx, Zhinx, Zhinxmin codegen

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D122918#3762609 <https://reviews.llvm.org/D122918#3762609>, @craig.topper wrote:

> In D122918#3762574 <https://reviews.llvm.org/D122918#3762574>, @reames wrote:
>
>> In D122918#3762537 <https://reviews.llvm.org/D122918#3762537>, @craig.topper wrote:
>>
>>> Assembler and disassembler support was already commited in https://reviews.llvm.org/D93298
>>
>> Then why does this contain what look to be MC related changes to .td files?  Looking closer, I do see that something has already landed, but there's definitely MC pieces in this.  Confusing to say the least.
>
> Can you point to a specific example?

I think I was seeing the FLD_IN32X definition from the Aug 28th version of the patch.  This has since been removed, and even at the time of my comment, I must have been looking at a stale diff.  Sorry for the noise.

Two minor optional style comments.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:12806
   case MVT::f16:
-    return Subtarget.hasStdExtZfh();
+    return Subtarget.hasStdExtZfh() || Subtarget.hasStdExtZhinx();
   case MVT::f32:
----------------
This pattern repeats a lot.  Maybe worth introducing a "Subtarget.hasStdExtZfhorZhinx()" cover?

Also for corresponding F/D variants.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td:474
+// half->[u]int. Round-to-zero must be used.
+def : Pat<(i32 (any_fp_to_sint FPR16INX:$rs1)), (FCVT_W_H_INX $rs1, 0b001)>;
+def : Pat<(i32 (any_fp_to_uint FPR16INX:$rs1)), (FCVT_WU_H_INX $rs1, 0b001)>;
----------------
For the rounding modes, defining symbolic constant names would make the code easier to read.  I see that this pattern exists in the current code, so this is a non-blocking suggestion.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122918