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

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

In D63433#1553128 <https://reviews.llvm.org/D63433#1553128>, @asb wrote:

> 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".


The first version of this patch took an optimistic view. It assumed that all IR operations could take a zero immediate (via the zero register) or a 12-bit immediate, and both of these were essentially "free". All other immediates were as expensive as materialising them would be (in terms of selectiondag nodes).

I am now taking a more conservative view. While I still assume that all operations can take a zero immediate (via the zero register) for free, I now assume all other immediate values are as expensive as materialising the integer, unless I have more information (like `isAddLike`).

I hoped this approach would work, but it turns out this hoists a lot of small constants (like `i64 1`). This doesn't sound like an issue, but it causes regressions in lots of the calling convention tests on rv32. I'm going to disable constant hoisting in the cases we don't know about.

I also had to switch off hoisting entirely for intrinsics because constant hoisting will gladly hoist an "expensive" argument, even if that intrinsic requires that argument is an immediate (via `ImmArg`). I am working on a separate patch for ConstantHoisting to prevent this issue.

> 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

I'm happy to introduce this switch block, including examining argument indexes to ensure they match the native instructions with immediates. This would indeed prevent hoisting where a complex operation might be lowered into simpler native instructions, but given the status before this patch is applied is that no hoisting is done at all, the patch cannot 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.

I'm not clear on this either. I can look at the method history.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63433