Page MenuHomePhabricator

[SVE] Make EVT::getScalarSizeInBits and others consistent with Type::getScalarSizeInBits
ClosedPublic

Authored by david-arm on Sep 18 2020, 1:32 AM.

Details

Summary

An existing function Type::getScalarSizeInBits returns a uint64_t
instead of a TypeSize class because the caller is requesting a
scalar size, which cannot be scalable. This patch makes other
similar functions requesting a scalar size consistent with that,
thereby eliminating more than 800 implicit TypeSize -> uint64_t
casts.

Diff Detail

Event Timeline

david-arm created this revision.Sep 18 2020, 1:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 1:32 AM
david-arm requested review of this revision.Sep 18 2020, 1:32 AM

I think this makes sense, but I'm interested to see if anyone else has a different opinion.

thereby eliminating more than 1000 implicit TypeSize -> uint64_t casts.

I'm curious, how many are there in total?

david-arm edited the summary of this revision. (Show Details)Sep 20 2020, 11:57 PM

Hi @efriedma, so it turns out it's over 800, not 1000. :) My initial guess was a bit too high, but here are the counts I found:

grep -R getScalarValueSizeInBits llvm/ | wc -l
149

grep -R getScalarSizeInBits llvm/lib/CodeGen/SelectionDAG/ | wc -l
214

grep -R getScalarSizeInBits llvm/lib/Target/ | wc -l
456

Of course, a small percentage of calls to getScalarSizeInBits could be to the existing IR Type class member function, but having looked through the results I think are most are for MVTs and EVTs.

sdesmalen accepted this revision.Tue, Sep 22, 9:44 AM

LGTM. I think this is a sensible change. TypeSize is used to indicate that a value could be scalable, but if it is known the type is always fixed-size this change simplifies code that calls getScalarSizeInBits (i.e. no need to call 'getFixedSize`).
It also puts us on the path towards removing the implicit casts to uint64_t.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7443–7444

s/OrigEltTy.getSizeInBits().getFixedSize()/OrigEltTy.getScalarSizeInBits()/ ?

llvm/lib/Target/ARM/ARMISelLowering.cpp
7848

s/OrigEltTy.getSizeInBits().getFixedSize()/OrigEltTy.getScalarSizeInBits()/ ?

This revision is now accepted and ready to land.Tue, Sep 22, 9:44 AM
This revision was landed with ongoing or failed builds.Wed, Sep 23, 1:20 AM
This revision was automatically updated to reflect the committed changes.
david-arm marked 2 inline comments as done.Wed, Sep 23, 1:22 AM