[PATCH] D15816: [TTI] Add hook for contextual cast estimates

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:441
@@ -440,1 +440,3 @@
 
+  /// A struct to hold information for contextual cost estimates.
+  struct InstrContext {
----------------
I feel that this structure should contain EITHER an anonymous opcode (as it currently does) OR a Value*.

I think it is not worthwhile to have the user reencode their use-def graph for the purposes of passing as context. What we need to be able to say is "I'm going to insert $OPCODE and it's going to use these values". Some of the values may be yet to be inserted - so these could be anonymous too. But encoding the entire use-def graph to an arbitrary depth sounds like wasted effort. The code below for AArch64 seems to look quite deep at the graph too.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:468
@@ +467,3 @@
+  /// \return The expected cost of a cast instruction in the given context.
+  int getCastInstrCostInCtx(InstrContext &Ctx) const;
+
----------------
I'd prefer if an optional Ctx argument were added to existing methods.

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:329
@@ +328,3 @@
+    auto *Src = dyn_cast_or_null<IntegerType>(Ctx.Operands[0].InstrTy);
+    assert(Dst && Src == Vec->getElementType() && "Invalid context");
+
----------------
If you expect Dst, you must use cast<> above instead of dyn_cast_or_null. Similarly in all the other place you use dyn_cast_or_null, I don't think a nullptr argument is *actually* allowed.


http://reviews.llvm.org/D15816