This removes from TargetRegisterClass the last piece of information that is going to be parametrized. All of the parametrized data will reside in TargetRegisterInfo.
Details
- Reviewers
qcolombet asb reames sdardis MatzeB craig.topper hfinkel - Commits
- rGc8e8e2a046f2: Move value type list from TargetRegisterClass to TargetRegisterInfo
rGc0197066d786: Move value type list from TargetRegisterClass to TargetRegisterInfo
rL301234: Move value type list from TargetRegisterClass to TargetRegisterInfo
rL301231: Move value type list from TargetRegisterClass to TargetRegisterInfo
Diff Detail
- Repository
- rL LLVM
Event Timeline
Nothing is new, but I forgot to add llvm-commits as a subscriber. Hopefully this update will cause an email to be sent to the list.
I had a couple of minor review comments. I know the use of pointers vs references is quite inconsistent across LLVM, but I do feel it would be an improvement if isLegalRC took a const TargetRegisterInfo& (though perhaps there's a reason not to do this that I'm missing?).
include/llvm/Target/TargetLowering.h | ||
---|---|---|
2265 ↗ | (On Diff #94829) | Given that TRI must be non-null in the isLegalRC implementation later on in this patch, perhaps this function should take a reference to TargetRegisterInfo? |
include/llvm/Target/TargetRegisterInfo.h | ||
335 ↗ | (On Diff #94829) | Nitpick: I know this is just copied from the old vt_end, but having ++I on the next line would be more usual LLVM coding style (and is what clang-format would produce). |
lib/CodeGen/TargetLoweringBase.cpp | ||
1189 ↗ | (On Diff #94829) | Even though it's slightly less typing, I can't help but feel that checking for MVT::Other is peeking under the iterator abstraction unnecessarily. |
lib/CodeGen/TargetLoweringBase.cpp | ||
---|---|---|
1189 ↗ | (On Diff #94829) | The reason I did it was that the end iterator traverses the whole list, also unnecessarily. We could have a separate class for the VT iterator, and implement something like "isValid" in it, but
With that in mind, I'm not sure that it's worth it. |
Thanks, looks good to me.
lib/CodeGen/TargetLoweringBase.cpp | ||
---|---|---|
1189 ↗ | (On Diff #94829) | Yes, I figured that was why. I did wonder if LLVM's codegen would be smart enough to optimise the repeated traversal. It's not an important point one way or the other anyway. |
Looks generally good to me, but I think we should rename the function when moving to register info (see below).
include/llvm/Target/TargetRegisterInfo.h | ||
---|---|---|
320 ↗ | (On Diff #95776) | The name TargetRegisterInfo::hasType feels odd as this isn't a general property of TargetRegisterInfo. Maybe rename to regclassHasType()? Similar with the next two functions (regclassTypes_begin() maybe?). |
include/llvm/Target/TargetRegisterInfo.h | ||
---|---|---|
320 ↗ | (On Diff #95776) | How about isTypeLegalForClass and regclassvt_begin, regclassvt_end? |
include/llvm/Target/TargetRegisterInfo.h | ||
---|---|---|
320 ↗ | (On Diff #95776) | I'd be fine with them independently. Though maybe there is a way to make the look more similar (as they are all about the same concept). isTypeLegalForClass(), legalClassTypesBegin(), legalClassTypesEnd()? |
include/llvm/Target/TargetRegisterInfo.h | ||
---|---|---|
320 ↗ | (On Diff #95776) | From what I've seen elsewhere, the convention for iterators is "lowercase with _begin/_end". Do legalclasstypes_begin and legalclasstypes_end look ok to you? |
include/llvm/Target/TargetRegisterInfo.h | ||
---|---|---|
320 ↗ | (On Diff #95776) | Guess it's llvm naming conventions clashing with STL ones, looks good. |