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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:650
+  return {{unsigned(*UnrolledCost.getValue()),
+           unsigned(*RolledDynamicCost.getValue())}};
 }
----------------
sdesmalen wrote:
> fhahn wrote:
> > sdesmalen wrote:
> > > 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.
> > Is it that uncommon? It looks like the majority of uses in `LoopVectorize.cpp` for example use `*Cost.getValue()` for example.
> That's only temporary, until we finish migrating places to work with InstructionCost directly, so most of these `*Cost.getValue()` statements will go away.
Ok, sounds like  it's probably not worth worrying about it too much then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95817