[PATCH] D91174: [Support] Introduce a new InstructionCost class

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7167
   case Instruction::ExtractValue:
-    return TTI.getInstructionCost(I, TTI::TCK_RecipThroughput);
+    return *(TTI.getInstructionCost(I, TTI::TCK_RecipThroughput).getValue());
   default:
----------------
RKSimon wrote:
> Is it a good idea to always "dereference" optionals like this?
In principle, "no". However, my assumption is that this function will eventually be changed to return InstructionCost. For now, all we can do is assert that it's valid.

a nice assert would be nice, but Optional::operator* does assert that there's a value, so a debugger will break in a useful location if this code ever tries to unwrap a none


================
Comment at: llvm/unittests/Support/InstructionCostTest.cpp:49
+  EXPECT_EQ(++VThree, 4);
+  EXPECT_EQ(VThree++, 4);
+  EXPECT_EQ(--VThree, 4);
----------------
The next line should be `EXPECT_EQ(VThree, 5);`, and vice versa for the postfix decrement. As it stands, somebody could break the behavior that postfix increment and decrement actually modify the value and these tests would still pass.


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

https://reviews.llvm.org/D91174