This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Use [MC]Register for Hexagon target
ClosedPublic

Authored by gjain on Nov 10 2020, 6:49 AM.

Diff Detail

Event Timeline

gjain created this revision.Nov 10 2020, 6:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 6:49 AM
gjain requested review of this revision.Nov 10 2020, 6:49 AM

Maybe the popular RegisterRef can be moved to Register.h? Would that complicate this patch?

llvm/lib/Target/Hexagon/BitTracker.h
134

are Reg and Pos const?

147

Are Reg and Sub const? marking them so would help readability, and also ensure we don't accidentally yank the initializer in the ctor.

llvm/lib/Target/Hexagon/HexagonBlockRanges.h
35

weird... there was one of these earlier. Not sure if hoisting it in a common .h may make sense, but otherwise the comments are the same (const-ing Reg and Sub)

llvm/lib/Target/Hexagon/HexagonConstPropagation.cpp
86–87

seems like a popular design

llvm/lib/Target/Hexagon/HexagonExpandCondsets.cpp
191–192

4th RegisterRef.

Sub is probably unsigned?

llvm/lib/Target/Hexagon/HexagonGenPredicate.cpp
51–52

5th RegisterRef :)

gjain marked 6 inline comments as done.Nov 10 2020, 3:32 PM

I'd definitely prefer not cleaning up RegisterRef as part of this patch. The register/sub tuple pattern is present throughout the code and needs an independant clean-up. Otherwise we risk this not being a NFC.

Regarding const usage, this will delete the implicit copy constructor. Given the earlier statement that those classes need an independant clean-up, are you okay punting on this?

llvm/lib/Target/Hexagon/BitTracker.h
134

See comments.

llvm/lib/Target/Hexagon/HexagonBlockRanges.h
35

Yeah this was very odd and requires clean-up

llvm/lib/Target/Hexagon/HexagonConstPropagation.cpp
86–87

See comments.

gjain updated this revision to Diff 304343.Nov 10 2020, 3:34 PM
gjain marked 3 inline comments as done.

Fix Sub to be unsigned

I'd definitely prefer not cleaning up RegisterRef as part of this patch. The register/sub tuple pattern is present throughout the code and needs an independant clean-up. Otherwise we risk this not being a NFC.

Regarding const usage, this will delete the implicit copy constructor. Given the earlier statement that those classes need an independant clean-up, are you okay punting on this?

Yes, but could you add a FIXME above each, with the same text - so we can fish them out easily afterwards.

mtrofin accepted this revision.Nov 10 2020, 8:40 PM

lgtm

This revision is now accepted and ready to land.Nov 10 2020, 8:40 PM
This revision was landed with ongoing or failed builds.Nov 18 2020, 8:19 AM
This revision was automatically updated to reflect the committed changes.