[PATCH] D63433: [RISCV] Add RISCV-specific TargetTransformInfo

[Alex Bradbury via Phabricator via llvm-commits]

asb added a subscriber: ributzka.
asb added a comment.

Thanks for the patch! I note that test/CodeGen/RISCV/{imm-cse.ll,split-offsets.ll} have codegen changes with this. imm-cse.ll is a neutral change but split-offsets.ll seems to be a regression in instruction count that merits investigation.

With that addressed I _think_ this is fine. One aspect I'm struggling to determine one way or the other is whether the behaviour you implement here of returning TCC_FREE for getIntImmCost(const APInt &Imm) for a 12-bit value (which can be merged into an ADDI) is correct, or whether that case should only be TCC_FREE if going through `getIntImmCost(unsigned Opcode, unsigned Idx, const APInt &Imm, Type *Ty);`. The PPC backend seems to handle it that way, while AArch64 handles it similar to how you do. From reading the getIntImmCost descriptions in TargetTransformInfo.h I probably would have assumed the PPC way was "most correct".

@ributzka - which behaviour did you intend when creating these APIs?



================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:9
+/// \file
+/// This file a TargetTransformInfo::Concept conforming object specific to the
+/// RISC-V target machine. It uses the target's detailed information to
----------------
"file a" is missing a word. Probably should be file contains a / file describes a / some other similar phrase


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:44
+
+  using BaseT::getIntImmCost;
+  int getIntImmCost(const APInt &Imm);
----------------
Maybe I'm missing some C++ magic or this is a compatibility thing, but I don't think this line is needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63433