[PATCH] D62857: [RISCV] Prevent re-ordering some adds after shifts

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for the update Sam, I think there might be a minor correctness issue with getIntMatCost - let me know what you think.

It would be great to add a simple sanity check for the chunking logic. Perhaps there's a test case involving `-1` that produces a different answer with the current version of the patch versus a version updated to use the type size for the bit size.



================
Comment at: llvm/lib/Target/RISCV/Utils/RISCVMatInt.cpp:78
+
+// This will attempt to produce instructions to materialise `Val`. `Is64Bit`
+// should match the target architecture.
----------------
Given you have a description in the header, I think this repeated description for the implementation is redundant (see https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments "Don’t duplicate the documentation comment in the header file and in the implementation file"). Though maybe I missed a de facto standard elsewhere in the codebase for duplicating a reduced version. I don't feel strongly about this, so do feel free to keep if you prefer.


================
Comment at: llvm/lib/Target/RISCV/Utils/RISCVMatInt.cpp:81
+int getIntMatCost(const APInt &Val, bool Is64Bit) {
+  unsigned BitSize = Val.getMinSignedBits();
+  int PlatRegSize = Is64Bit ? 64 : 32;
----------------
I think this isn't going to give the desired result if Val is e.g. -1. In that case, getMinSignedBits is going to return 1, meaning the loop is only executed once regardless of the original type width. If the original type was e.g. an i64 on RV32, then two instructions are required to materialise the constant, yet the logic in this function will only return 1.

I think the solutation is to more closely mirror the AArch64TTIImpl::getIntImmCost function I mentioned before, and add a Type parameter (and maybe also adopt similar logic for sign-extending constants to be a multiple of the PlatRegSize).


================
Comment at: llvm/lib/Target/RISCV/Utils/RISCVMatInt.h:41
+// should match the target architecture.
+int getIntMatCost(const APInt &Val, bool IsRV64);
 } // namespace RISCVMatInt
----------------
Nit: Shouldn't this be Is64Bit to to match the naming in the implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62857