[PATCH] D95817: NFC: Migrate LoopUnrollPass to work on InstructionCost

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:650
+  return {{unsigned(*UnrolledCost.getValue()),
+           unsigned(*RolledDynamicCost.getValue())}};
 }
----------------
fhahn wrote:
> should we add an accessor that returns the value directly? The extra dereferences look odd, especially because we already check that the cost is valid.
When @david-arm created the class I initially argued against that because I didn't want people to assume `getValue()` always returns a value, and crashes when the value isn't valid. I agree with you that the current interface is cumbersome in these cases, but fortunately this use-case is quite rare; in most places InstructionCost is either propagated, accumulated or compared directly.

I could add some `getValidValue()` where the name hints the cost must be valid (and where the method asserts it is), but given the limited use-cases in practice, I think I still prefer to stick to the current interface where it returns an Optional, because that makes it unequivocally clear the value may not be set if the InstructionCost is invalid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95817