This is an archive of the discontinued LLVM Phabricator instance.

Generate `MachineValueType.h` (partially) from `ValueTypes.td`
ClosedPublic

Authored by chapuni on Mar 26 2023, 3:37 AM.

Details

Summary
  • Implement VTEmitter as llvm-tblgen -gen-vt.
  • Create a copy of llvm/Support/MachineValueType.h into unittests/Support. It includes GenVT.inc generated by VTEmitter.
  • Implement MVTTest in SupportTests. It checks equivalence between llvm/Support/MachineValueType.h and the generated header.

Depends on D146907

Diff Detail

Event Timeline

chapuni created this revision.Mar 26 2023, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 3:37 AM
chapuni requested review of this revision.Mar 26 2023, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 3:37 AM
chapuni updated this revision to Diff 508397.Mar 26 2023, 3:57 AM
chapuni edited the summary of this revision. (Show Details)
  • Split out D146907 (as a reference)
chapuni added inline comments.Mar 26 2023, 4:12 AM
llvm/utils/TableGen/VTEmitter.cpp
82

They have different names between MachineValueType.h and ValueTypes.td.
Could we unify, or give alias names for them? (See also D146179)

chapuni updated this revision to Diff 508398.Mar 26 2023, 4:33 AM
  • Fixup includes
barannikov88 added a subscriber: barannikov88.EditedMar 26 2023, 9:43 AM

This wont work as llvm-tblgen depends on MachineValueType.h.

@barannikov88 Could you see D146352 and the series of D145937?
I will create a small tablegen (including this) w/o MVT.h.

@barannikov88 Could you see D146352 and the series of D145937?
I will create a small tablegen (including this) w/o MVT.h.

That's a big patch series. Could you point me to the RFC?

Thanks, now I see the bigger picture.
Please disregard my comment above as it was made out of full context.

PS
It would be nice if the RFC link was mentioned in the phab's description of the first commit in the stack.
(Or maybe even in the description of every commit in the series.)

chapuni updated this revision to Diff 509664.Mar 30 2023, 7:14 AM
  • Update Bazel
arsenm added inline comments.Mar 30 2023, 11:10 AM
llvm/unittests/Support/MVTTest.cpp
28–29

auto is longer than MVT, just declare the type

llvm/utils/TableGen/VTEmitter.cpp
40

You never resize this, use std::array

56

DenseMap?

chapuni updated this revision to Diff 509850.Mar 30 2023, 4:35 PM
  • Use std::array
  • Update iwyu
chapuni marked an inline comment as done.Mar 30 2023, 4:57 PM
chapuni added inline comments.
llvm/unittests/Support/MVTTest.cpp
28–29

Each RHS is the constructor of MVT and either is tmp::MVT. Could I rewrite both of them?

llvm/utils/TableGen/VTEmitter.cpp
56

VTRanges is accessed by iterator. See L93 and D146179.

chapuni updated this revision to Diff 509875.Mar 30 2023, 6:44 PM
  • Fix formatting
  • Rewind isFixedLengthVector(), since FIRST/LAST_FIXEDLEN_VECTOR_VALUETYPE will come from ValueTypes.td
chapuni updated this revision to Diff 512133.Apr 10 2023, 6:34 AM
  • Prune garbage

Ping.

@arsenm Any other concerns?

arsenm accepted this revision.Apr 13 2023, 3:37 PM
This revision is now accepted and ready to land.Apr 13 2023, 3:37 PM
nemanjai added inline comments.
llvm/utils/TableGen/VTEmitter.cpp
79

I don't really follow the logic here. Between this code and the subsequent reorganization of MachineValueTypes.h to generate the MVT, what meaning does an enumerator such as LAST_VALUETYPE have?
Prior to this, there was a sequence of consecutive VT's and LAST_VALUTYPE was set to the last one. As people add more, if they exceed MAX_ALLOWED_VALUETYPE, the static_assert will let them know.

With this reorganization, if someone defines types number 224, 225, etc. there will be no compile issue and LAST_VALUETYPE will be set to whatever number 223 is - everything else will presumably be ignored by the legalizer or whatever else depends on this. Is this really the intended behaviour?

chapuni added inline comments.Sun, Nov 26, 12:34 AM
llvm/utils/TableGen/VTEmitter.cpp
79

It came from my easy assumption just for the consistency. It should be refactored if MAX_ALLOWED_VALUETYPE is increased.

Sorry for the delay.