[RISCV] Add RISCV-specific TargetTransformInfo

[lenary]

Summary

LLVM Allows Targets to provide information that guides optimisations
made to LLVM IR. This is done with callbacks on a TargetTransformInfo object.

This patch adds a TargetTransformInfo class for RISC-V. This will allow us to
implement RISC-V specific callbacks as they become necessary.

This commit also adds the getIntImmCost callbacks, and tests them with a simple
constant hoisting test.

Previous review was on D63007

[lenary]

This is blocked on D62857

[lenary]

• Address D63007 feedback from @MaskRay

[asb]

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?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h

9 ↗

(On Diff #205085)

"file a" is missing a word. Probably should be file contains a / file describes a / some other similar phrase

44 ↗

(On Diff #205085)

Maybe I'm missing some C++ magic or this is a compatibility thing, but I don't think this line is needed?

[luismarques]

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

29 ↗

(On Diff #205085)

You can add an assert message. E.g. assert(foo && "some message").

[lenary]

Address review feedback:

• Return TCC_Free in many more unknown cases, to prevent constant hoisting
• Only return TCC_Free for 12-bit immediates in arithmetic operations
• Use RISCVMatInt::getIntMatCost to accurately estimate materialisation cost
• Remove some useless lines and update comments and asserts.

[ributzka]

If I recall correctly, then getIntImmCost(const APInt &Imm) predates the ConstantHoisting pass and is also used by other optimizations. The purpose of that hook is to calculate the materialization cost of a constant and is context free.

I added getIntImmCost(unsigned Opcode/Intrinsic::ID IID, unsigned Idx, const APInt &Imm, Type *Ty), because many instructions on AArch64 were able to fold constants and getIntImmCost(const APInt &Imm) would not provide accurate information to identify expensive constants.

[asb]

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.

[lenary]

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

[lenary]

Address Review Comments:

• Make getIntMatCost significantly more conservative, to avoid regressions.
• Make getIntMatCost prevent hoisting for all intrinsics to avoid a bug.

[asb]

LGTM, thanks!

llvm/trunk/lib/Target/RISCV/CMakeLists.txt

add_llvm_target(RISCVCodeGen
   RISCVISelDAGToDAG.cpp
   RISCVISelLowering.cpp
   RISCVMCInstLower.cpp
   RISCVMergeBaseOffset.cpp
   RISCVRegisterInfo.cpp
   RISCVSubtarget.cpp
   RISCVTargetMachine.cpp
   RISCVTargetObjectFile.cpp
+  RISCVTargetTransformInfo.cpp
   )
 
 add_subdirectory(AsmParser)
 add_subdirectory(Disassembler)
 add_subdirectory(MCTargetDesc)
 add_subdirectory(TargetInfo)
 add_subdirectory(Utils)

llvm/trunk/lib/Target/RISCV/LLVMBuild.txt

 has_asmparser = 1
 has_asmprinter = 1
 has_disassembler = 1
 
 [component_1]
 type = Library
 name = RISCVCodeGen
 parent = RISCV
-required_libraries = AsmPrinter Core CodeGen MC RISCVDesc
+required_libraries = Analysis AsmPrinter Core CodeGen MC RISCVDesc
   RISCVInfo RISCVUtils SelectionDAG Support Target
 add_to_library_groups = RISCV

llvm/trunk/lib/Target/RISCV/RISCVTargetMachine.h

const RISCVSubtarget *getSubtargetImpl(const Function &) const override {
     return &Subtarget;
   }
 
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
+
+  TargetTransformInfo getTargetTransformInfo(const Function &F) override;
 };
 }
 
 #endif

llvm/trunk/lib/Target/RISCV/RISCVTargetMachine.cpp

 //===-- RISCVTargetMachine.cpp - Define TargetMachine for RISCV -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Implements the info about RISCV target spec.
 //
 //===----------------------------------------------------------------------===//
 
-#include "RISCV.h"
 #include "RISCVTargetMachine.h"
+#include "RISCV.h"
 #include "RISCVTargetObjectFile.h"
+#include "RISCVTargetTransformInfo.h"
 #include "TargetInfo/RISCVTargetInfo.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
RISCVTargetMachine::RISCVTargetMachine(const Target &T, const Triple &TT,
     : LLVMTargetMachine(T, computeDataLayout(TT), TT, CPU, FS, Options,
                         getEffectiveRelocModel(TT, RM),
                         getEffectiveCodeModel(CM, CodeModel::Small), OL),
       TLOF(make_unique<RISCVELFTargetObjectFile>()),
       Subtarget(TT, CPU, FS, Options.MCOptions.getABIName(), *this) {
   initAsmInfo();
 }
 
+TargetTransformInfo
+RISCVTargetMachine::getTargetTransformInfo(const Function &F) {
+  return TargetTransformInfo(RISCVTTIImpl(this, F));
+}
+
 namespace {
 class RISCVPassConfig : public TargetPassConfig {
 public:
   RISCVPassConfig(RISCVTargetMachine &TM, PassManagerBase &PM)
       : TargetPassConfig(TM, PM) {}
 
   RISCVTargetMachine &getRISCVTargetMachine() const {
     return getTM<RISCVTargetMachine>();

llvm/trunk/lib/Target/RISCV/RISCVTargetTransformInfo.h

+//===- RISCVTargetTransformInfo.h - RISC-V specific TTI ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+/// \file
+/// This file defines a TargetTransformInfo::Concept conforming object specific
+/// to the RISC-V target machine. It uses the target's detailed information to
+/// provide more precise answers to certain TTI queries, while letting the
+/// target independent and default TTI implementations handle the rest.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_RISCV_RISCVTARGETTRANSFORMINFO_H
+#define LLVM_LIB_TARGET_RISCV_RISCVTARGETTRANSFORMINFO_H
+
+#include "RISCVSubtarget.h"
+#include "RISCVTargetMachine.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
+#include "llvm/CodeGen/BasicTTIImpl.h"
+#include "llvm/IR/Function.h"
+
+namespace llvm {
+
+class RISCVTTIImpl : public BasicTTIImplBase<RISCVTTIImpl> {
+  using BaseT = BasicTTIImplBase<RISCVTTIImpl>;
+  using TTI = TargetTransformInfo;
+
+  friend BaseT;
+
+  const RISCVSubtarget *ST;
+  const RISCVTargetLowering *TLI;
+
+  const RISCVSubtarget *getST() const { return ST; }
+  const RISCVTargetLowering *getTLI() const { return TLI; }
+
+public:
+  explicit RISCVTTIImpl(const RISCVTargetMachine *TM, const Function &F)
+      : BaseT(TM, F.getParent()->getDataLayout()), ST(TM->getSubtargetImpl(F)),
+        TLI(ST->getTargetLowering()) {}
+
+  int getIntImmCost(const APInt &Imm, Type *Ty);
+  int getIntImmCost(unsigned Opcode, unsigned Idx, const APInt &Imm, Type *Ty);
+  int getIntImmCost(Intrinsic::ID IID, unsigned Idx, const APInt &Imm,
+                    Type *Ty);
+};
+
+} // end namespace llvm
+
+#endif // LLVM_LIB_TARGET_RISCV_RISCVTARGETTRANSFORMINFO_H
 No newline at end of file

llvm/trunk/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

+//===-- RISCVTargetTransformInfo.cpp - RISC-V specific TTI ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RISCVTargetTransformInfo.h"
+#include "Utils/RISCVMatInt.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
+#include "llvm/CodeGen/BasicTTIImpl.h"
+#include "llvm/CodeGen/TargetLowering.h"
+using namespace llvm;
+
+#define DEBUG_TYPE "riscvtti"
+
+int RISCVTTIImpl::getIntImmCost(const APInt &Imm, Type *Ty) {
+  assert(Ty->isIntegerTy() &&
+         "getIntImmCost can only estimate cost of materialising integers");
+
+  // We have a Zero register, so 0 is always free.
+  if (Imm == 0)
+    return TTI::TCC_Free;
+
+  // Otherwise, we check how many instructions it will take to materialise.
+  const DataLayout &DL = getDataLayout();
+  return RISCVMatInt::getIntMatCost(Imm, DL.getTypeSizeInBits(Ty),
+                                    getST()->is64Bit());
+}
+
+int RISCVTTIImpl::getIntImmCost(unsigned Opcode, unsigned Idx, const APInt &Imm,
+                                Type *Ty) {
+  assert(Ty->isIntegerTy() &&
+         "getIntImmCost can only estimate cost of materialising integers");
+
+  // We have a Zero register, so 0 is always free.
+  if (Imm == 0)
+    return TTI::TCC_Free;
+
+  // Some instructions in RISC-V can take a 12-bit immediate. Some of these are
+  // commutative, in others the immediate comes from a specific argument index.
+  bool Takes12BitImm = false;
+  unsigned ImmArgIdx = ~0U;
+
+  switch (Opcode) {
+  case Instruction::GetElementPtr:
+    // Never hoist any arguments to a GetElementPtr. CodeGenPrepare will
+    // split up large offsets in GEP into better parts than ConstantHoisting
+    // can.
+    return TTI::TCC_Free;
+  case Instruction::Add:
+  case Instruction::And:
+  case Instruction::Or:
+  case Instruction::Xor:
+  case Instruction::Mul:
+    Takes12BitImm = true;
+    break;
+  case Instruction::Sub:
+  case Instruction::Shl:
+  case Instruction::LShr:
+  case Instruction::AShr:
+    Takes12BitImm = true;
+    ImmArgIdx = 1;
+    break;
+  default:
+    break;
+  }
+
+  if (Takes12BitImm) {
+    // Check immediate is the correct argument...
+    if (Instruction::isCommutative(Opcode) || Idx == ImmArgIdx) {
+      // ... and fits into the 12-bit immediate.
+      if (getTLI()->isLegalAddImmediate(Imm.getSExtValue()))
+        return TTI::TCC_Free;
+    }
+
+    // Otherwise, use the full materialisation cost.
+    return getIntImmCost(Imm, Ty);
+  }
+
+  // By default, prevent hoisting.
+  return TTI::TCC_Free;
+}
+
+int RISCVTTIImpl::getIntImmCost(Intrinsic::ID IID, unsigned Idx,
+                                const APInt &Imm, Type *Ty) {
+  // Prevent hoisting in unknown cases.
+  return TTI::TCC_Free;
+}

llvm/trunk/test/CodeGen/RISCV/imm-cse.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
 ; RUN:   | FileCheck %s -check-prefix=RV32I
 
 @src = global i32 0
 @dst = global i32 0
 
 ; Tests that the common hi20 value (1) for the constants is used rather than
 ; redundantly re-materialised.
 define void @imm32_cse() nounwind {
 ; RV32I-LABEL: imm32_cse:
 ; RV32I:       # %bb.0:
 ; RV32I-NEXT:    lui a0, 1
-; RV32I-NEXT:    addi a1, a0, 1
-; RV32I-NEXT:    lui a2, %hi(src)
-; RV32I-NEXT:    lw a3, %lo(src)(a2)
-; RV32I-NEXT:    add a1, a3, a1
+; RV32I-NEXT:    addi a0, a0, 1
+; RV32I-NEXT:    lui a1, %hi(src)
+; RV32I-NEXT:    lw a2, %lo(src)(a1)
+; RV32I-NEXT:    add a2, a2, a0
 ; RV32I-NEXT:    lui a3, %hi(dst)
-; RV32I-NEXT:    sw a1, %lo(dst)(a3)
-; RV32I-NEXT:    addi a1, a0, 2
-; RV32I-NEXT:    lw a4, %lo(src)(a2)
-; RV32I-NEXT:    add a1, a4, a1
-; RV32I-NEXT:    sw a1, %lo(dst)(a3)
-; RV32I-NEXT:    addi a0, a0, 3
-; RV32I-NEXT:    lw a1, %lo(src)(a2)
+; RV32I-NEXT:    sw a2, %lo(dst)(a3)
+; RV32I-NEXT:    lw a2, %lo(src)(a1)
+; RV32I-NEXT:    add a2, a2, a0
+; RV32I-NEXT:    addi a2, a2, 1
+; RV32I-NEXT:    sw a2, %lo(dst)(a3)
+; RV32I-NEXT:    lw a1, %lo(src)(a1)
 ; RV32I-NEXT:    add a0, a1, a0
+; RV32I-NEXT:    addi a0, a0, 2
 ; RV32I-NEXT:    sw a0, %lo(dst)(a3)
 ; RV32I-NEXT:    ret
   %1 = load volatile i32, i32* @src
   %2 = add i32 %1, 4097
   store volatile i32 %2, i32* @dst
   %3 = load volatile i32, i32* @src
   %4 = add i32 %3, 4098
   store volatile i32 %4, i32* @dst
   %5 = load volatile i32, i32* @src
   %6 = add i32 %5, 4099
   store volatile i32 %6, i32* @dst
   ret void
 }

llvm/trunk/test/Transforms/ConstantHoisting/RISCV/immediates.ll

+; RUN: opt -mtriple=riscv32-unknown-elf -S -consthoist < %s | FileCheck %s
+; RUN: opt -mtriple=riscv64-unknown-elf -S -consthoist < %s | FileCheck %s
+
+; Check that we don't hoist immediates with small values.
+define i64 @test1(i64 %a) nounwind {
+; CHECK-LABEL: test1
+; CHECK-NOT: %const = bitcast i64 2 to i64
+  %1 = mul i64 %a, 2
+  %2 = add i64 %1, 2
+  ret i64 %2
+}
+
+; Check that we don't hoist immediates with small values.
+define i64 @test2(i64 %a) nounwind {
+; CHECK-LABEL: test2
+; CHECK-NOT: %const = bitcast i64 2047 to i64
+  %1 = mul i64 %a, 2047
+  %2 = add i64 %1, 2047
+  ret i64 %2
+}
+
+; Check that we hoist immediates with large values.
+define i64 @test3(i64 %a) nounwind {
+; CHECK-LABEL: test3
+; CHECK: %const = bitcast i64 32767 to i64
+  %1 = mul i64 %a, 32767
+  %2 = add i64 %1, 32767
+  ret i64 %2
+}
 No newline at end of file

llvm/trunk/test/Transforms/ConstantHoisting/RISCV/lit.local.cfg

+if not 'RISCV' in config.root.targets:
+    config.unsupported = True
 No newline at end of file