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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau accepted this revision.
ctetreau added a comment.
This revision is now accepted and ready to land.

Aside from the nit, this looks good to me.



================
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)
----------------
sdesmalen wrote:
> 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.
> If TotalMatCost is valid, then TotalFoldedCost must be valid as well, by the assert above.

If `TotalFoldedCost` and `TotalMatCost` are both invalid, and both have the same value in the `Value` field, then an invalid `TotalFoldingCost` can survive the assert on line 329. It would assert on the next line, so in practice it wouldn't be an issue, but code changes could lead to issues.

Looking at the code, the comparators of `InstructionCost` seem to be messed up. `operator==` considers the `Value` field and the `State` field in all cases, but `operator<` does not consider the `Value` field if either the rhs or lhs are invalid. We should probably fix this.

> a constant should always be materializable and if it is materializable, it should also be costable.

Then this assert should be above the other. This one states that constants can be materialized. The next one uses the cost of materialized constants.

Sorry if this all sounds super nitpicky. I'm just trying to understand the situation. I think reordering the asserts would help future readers understand the logic, but this is just a NIT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95353