[PATCH] D152421: [RISCV] Introduce the concept of DLEN(data path width) into getLMULCost.

Mon Jun 12 07:57:02 PDT 2023

reames added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:798

+def TuneDLenFactor2
+   : SubtargetFeature<"dlen-factor-2", "DLenFactor2", "true",
----------------
Can you add a comment here which defines DLEN?  

As an organizing principle, I was a bit surprised by the factor description.  I was expecting something more along the lines of VLEN (zvlNb) where we have specific sizes and then computed the factor from VLEN/DLEN if needed.  Not objecting, just commenting.

================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:822
+                                    TuneShortForwardBranchOpt,
+                                    TuneDLenFactor2]>;

----------------
The default tuning is used for more than the x280, are you sure you want this placement?

================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:182

+  unsigned getDLenFactor() const {
+    if (DLenFactor2)
----------------
asb wrote:
> I think we need a comment here to explain the purpose of the method.
Needs a comment, and/or a link back to the DLEN definition.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152421