This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Remove invalid TypeSize conversion from PromoteIntRes_BITCAST.
ClosedPublic

Authored by paulwalker-arm on Jun 6 2022, 9:41 AM.

Details

Summary

Extend the TypeWidenVector case of PromoteIntRes_BITCAST to work
with TypeSize directly rather than silently casting to unsigned.

To accomplish this I've extended TypeSize with an interface that
essentially allows TypeSize division when both operands have the
same number of dimensions.

There still exists combinations of scalable vector bitcasts that
cause compiler crashes. I call these out by adding "is missing"
entries to sve-bitcast.

Depends on D126957.
Fixes: #55114

Diff Detail

Event Timeline

paulwalker-arm created this revision.Jun 6 2022, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 9:41 AM
paulwalker-arm requested review of this revision.Jun 6 2022, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 9:41 AM
This revision is now accepted and ready to land.Jun 6 2022, 11:28 AM
This revision was landed with ongoing or failed builds.Jun 8 2022, 2:31 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen added inline comments.Jun 30 2022, 11:57 PM
llvm/include/llvm/Support/TypeSize.h
378

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.

385

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.

Discussed offline but for completeness.

llvm/include/llvm/Support/TypeSize.h
378

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.

385

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?