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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for the update Sam. In the previous version of the patch, `int getIntImmCost(unsigned Opcode, unsigned Idx, const APInt &Imm, Type *Ty);` just called `int getIntImmCost(const APInt &Imm, Type *Ty)`, but this one adds some logic to the former function that makes an estimate based on RISC-V instruction set knowledge. Right now you're ignoring the `Idx` argument and assuming that all binops can take a 12-bit immediate. Did you consider adding similar logic to the PPC and AArch64 versions of this function, having the switch setting ImmIdx appropriately and then using that to determine the cost? It looks like it wouldn't be much more complex or lengthy than what you have no to have a small `switch(...)` for the opcodes, and would have the advantage that `getIntImmCost` would be "done".

One difficulty I have with this code is I don't fully understand why other backend implementations are written as they are:

- Defaulting to `TCC_FREE` seems at first look needlessly pessimistic when it's not that hard to enumerate the operations which have native instructions with immediates. Though perhaps the thinking is that some of the more complex opcodes would be decomposed to simpler native instructions that have immediates, and so defaulting to constant hoisting would be a regression
- It's unclear why AArch64's implementation assumes mul/div/rem can take an immediate argument, given afaik the native instructions are all reg-reg.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63433