[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:
----------------
ctetreau wrote:
> 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
I suppose maybe we ought to be setting a good example with the introduction of this new type. If it always unconditionally dereferences the Optional from day one, monkey-see-monkey-do will ensure that this is done everywhere.

I think adding an actual assert, optionally  a FIXME would be good enough.


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

https://reviews.llvm.org/D91174