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

[David Sherwood via Phabricator via llvm-commits]

david-arm marked an inline comment as done.
david-arm 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:
> 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.
Hi @RKSimon, yeah I already have another patch (D92178) that changes this function to return a InstructionCost class so this dereference will disappear. That patch needs a bit more work after review comments and also needs rebasing to pick this up. However, sure for now I can add an assert if that helps! It's kind of like swings and roundabouts - checking in lots of smaller patches to migrate to using InstructionCost makes it easier for reviewers and easier to pin point any potential post-commit build failures. However, as a consequence it also means that interim measures like the dereference are needed sometimes.


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

https://reviews.llvm.org/D91174