[PATCH] D127126: [SelectionDAG] Remove invalid TypeSize conversion from PromoteIntRes_BITCAST.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Discussed offline but for completeness.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:378
+  /// will result in a value whose size matches our own.
+  bool hasKnownScalarFactor(const LinearPolySize &RHS) const {
+    return isScalable() == RHS.isScalable() &&
----------------
sdesmalen wrote:
> This seems to be roughly doing the same thing as `isKnownMultipleOf` from `LinearPolySize`. Should these interfaces be merged? If so, I prefer `isKnownMultipleOf` because it's a more intuitive name.
I don't believe `isKnownMultipleOf` is a good fit because `LinearPolySize.isKnownMultipleOf(LinearPolySize)` isn't necessarily a scalar[1] and so I felt an explicit function was safest and more readable.  If you have better names or function descriptions then please change them.

[1] I think the same argument holds for `getCommonFactor`.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:385
+  /// value whose size matches our own.
+  ScalarTy getKnownScalarFactor(const LinearPolySize &RHS) const {
+    assert(hasKnownScalarFactor(RHS) && "Expected RHS to be a known factor!");
----------------
sdesmalen wrote:
> It  might actually be nice if this could return an Optional, so that you could write:
> 
>   if (Optional<unsigned> F = EC.getCommonFactor(OtherEC)) {
>     // do something with F
>   }
> 
> bikeshedding: `getCommonFactor` would have been a better name IMO, even if it's perhaps less accurate.
Here I just followed the same logic as used by `isKnownMultipleOf` and `divideCoefficientBy`.  It's sometimes the case that the `is` function is embed within a multi condition `if` block so think using `Optional` might mess that up?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127126