[PATCH] D95353: NFC: Migrate SpeculateAroundPHIs to work on InstructionCost

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp:329
                                             "the sum must be as well.");
-
+  assert(TotalMatCost.isValid() && "Constants must be  materializable");
   LLVM_DEBUG(dbgs() << "    Cost savings " << (TotalMatCost - TotalFoldedCost)
----------------
ctetreau wrote:
> sdesmalen wrote:
> > ctetreau wrote:
> > > I assume this assert is to keep this patch NFC? If so, please reword the message to imply that it should be possible for this to happen, but it's not implemented yet or add a FIXME comment.
> > Not entirely; if a Constant cannot be folded when speculating, it must be possible to materialize it fully, and so we must be able to cost it. Otherwise there is something wrong with the existing IR. I don't think we should ever encounter this case.
> If a user writes bad IR, the system shouldn't crash. If this is truly impossible except in cases of bad IR, then we should raise a fatal error.
Perhaps I should have said: a constant should always be materializable and if it is materializable, it should also be costable. If that is not the case, the cost-model in the compiler is broken and needs fixing, not this pass.

If TotalMatCost is valid, then TotalFoldedCost must be valid as well, by the assert above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95353