[TableGen] Extend direct lookup to instruction values in generic tables. #80486

nvjle · 2024-02-02T20:09:34Z

Currently, for some tables involving a single primary key field which is integral and densely numbered, a direct lookup is generated rather than a binary search. This patch extends the direct lookup function generation to instructions, where the integral value corresponds to the instruction's enum value.

While this isn't as common as for other tables, it does occur in at least one downstream backend and one in-tree backend.

Added a unit test and minimally updated the documentation.

Currently, for some tables involving a single primary key field which is integral and densely numbered, a direct lookup is generated rather than a binary search. This patch extends the direct lookup function generation to instructions, where the integral value corresponds to the instruction's enum value. While this isn't as common as for other tables, it does occur in at least one downstream backend and one in-tree backend. Added a unit test and minimally updated the documentation.

nvjle · 2024-02-02T20:12:53Z

If one of @wangpc-pp , @nhaehnle , @Paul-C-Anagnostopoulos , or @TNorthover could review this small patch, it would be appreciated (chosen based on recent or past commits in this area-- other suggestions welcome). Thank you.

wangpc-pp

I think this is a great idea generally!

wangpc-pp · 2024-02-04T03:36:14Z

llvm/utils/TableGen/SearchableTableEmitter.cpp

+  if (SawInstructionField && InstrEnumValueMap.empty()) {
+    CodeGenTarget Target(Records);
+    unsigned Num = 0;
+    for (const CodeGenInstruction *Inst : Target.getInstructionsByEnumValue())


I think we should add a CodeGenTarget::getInstrToIntMap (or other names?) to get this instr->int mapping, so that we can reuse these logics and sync with other emittters (like you said, InstrInfoEmitter::emitEnums).

As you suggested, I have moved this map into CodeGenTarget::InstrToIntMap and added a lookup method CodeGenTarget::getInstrIntValue. The map is now populated in CodeGenTarget::computeInstrsByEnum right after the sort that creates the order in the first place.

Overall this is cleaner and simplified the patch as well. Thanks again for the suggestions.

In a follow-up patch, as you alluded to, I could use the new query in InstrInfoEmitter::emitEnums (and elsewhere if applicable) to reuse/sync the logic.

wangpc-pp · 2024-02-04T03:36:36Z

llvm/utils/TableGen/SearchableTableEmitter.cpp

@@ -638,6 +662,7 @@ void SearchableTableEmitter::collectTableEntries(

  Record *IntrinsicClass = Records.getClass("Intrinsic");
  Record *InstructionClass = Records.getClass("Instruction");
+  bool SawInstructionField = false;


SawInstructionField->HasInstructionField?

I have removed this variable since it is no longer necessary due to having moved the map population logic.

wangpc-pp · 2024-02-04T03:44:52Z

llvm/utils/TableGen/SearchableTableEmitter.cpp

@@ -207,12 +213,17 @@ class SearchableTableEmitter {

 // For search indices that consists of a single field whose numeric value is
 // known, return that numeric value.
-static int64_t getNumericKey(const SearchIndex &Index, Record *Rec) {
+static int64_t getNumericKey(const SearchIndex &Index, Record *Rec,
+                             InstrEnumMapT &InstrEnumMap) {


A style nit(just mine). What about making getNumericKey a class function like getIntrinsic, so that we don't need to pass InstrEnumMap?

Done. Although I removed the InstrEnumMap parameter, it is still useful to do this since we need to invoke the new query function Target->getInstrIntValue.

Move the instruction-to-int map into CodeGenTarget::InstrToIntMap and add CodeGenTarget::getInstrIntValue query method. Populate the map in CodeGenTarget::ComputeInstrsByEnum just below where the order is created in the first place.

nvjle · 2024-02-04T21:38:56Z

I think this is a great idea generally!

Thank you for the review comments, it is appreciated. I've made changes according to your suggestions below.

wangpc-pp

LGTM with nits.

wangpc-pp · 2024-02-05T04:54:53Z

llvm/utils/TableGen/SearchableTableEmitter.cpp

@@ -663,6 +682,8 @@ void SearchableTableEmitter::run(raw_ostream &OS) {
  // Emit tables in a deterministic order to avoid needless rebuilds.
  SmallVector<std::unique_ptr<GenericTable>, 4> Tables;
  DenseMap<Record *, GenericTable *> TableMap;
+  if (Records.getClass("Instruction"))


Suggested change

if (Records.getClass("Instruction"))

if (!Records.getAllDerivedDefinitions("Instruction").empty())

Do this only if there are Instruction definitions instead of Instruction class definition.

Done.
I've added the derived definitions guard as suggested. I needed to leave in the original guard since the derived definitions lookup will fail with "The class 'Instruction' is not defined" for clients that do not happen to have a Instruction class at all.

If I could also trouble you (or anyone else) to merge on my behalf, I'd appreciate that.

Oh sorry, I forgot it, we can use getAllDerivedDefinitionsIfDefined here.

If I could also trouble you (or anyone else) to merge on my behalf, I'd appreciate that.

Yeah, no problem!

Done (used getAllDerivedDefinitionsIfDefined).

Add derived definitions guard.

Use getAllDerivedDefinitionsIfDefined.

topperc · 2024-02-08T00:50:22Z

llvm/utils/TableGen/CodeGenTarget.cpp

+  // InstrInfoEmitter::emitEnums.
+  assert(InstrToIntMap.empty());
+  unsigned Num = 0;
+  for (const CodeGenInstruction *Inst : InstrsByEnum)


Sorry I'm late to this, but why not make this a field of CodeGenInstruction? We already have a map from Record* to CodeGenInstruction.

Sorry I'm late to this, but why not make this a field of CodeGenInstruction? We already have a map from Record to CodeGenInstruction.

Thanks for your comments @topperc, much appreciated. I'll make your suggested change in a new PR shortly, with you as reviewer/committer if you wouldn't mind.

piotrAMD · 2024-02-26T11:20:59Z

Looks the code assumes that Index.Fields[0].Enum and Index.Fields[0].IsInstruction are exclusive.

This is not universally true. For example WMMAOpcode2AddrMappingTable in AMDGPU target has both of them set.

This can cause problems, because SearchableTableEmitter::compareBy() or primaryRepresentation() check IsInstruction first and Enum next, but SearchableTableEmitter::getNumericKey() checks Enum first and IsInstruction next.

An immediate fix for that issue would be to switch the conditions around in getNumericKey so it checks Instruction before checking Enum maintainig consistency. Not sure how future-proof that would be, though.

piotrAMD · 2024-02-27T07:59:58Z

CC @nvjle (not sure if notifications are sent without explicit mention)

nvjle · 2024-02-27T15:46:47Z

CC @nvjle (not sure if notifications are sent without explicit mention)

Thanks @piotrAMD, I'll look into this in detail sometime this week. At first glance it does seem there may be a potential problem, but there is no actual failure that I can see. All testing is good thus far, including for AMDGPU. But we surely want this to be bullet proof.

piotrAMD · 2024-02-27T15:54:12Z

CC @nvjle (not sure if notifications are sent without explicit mention)

Thanks @piotrAMD, I'll look into this in detail sometime this week. At first glance it does seem there may be a potential problem, but there is no actual failure that I can see. All testing is good thus far, including for AMDGPU. But we surely want this to be bullet proof.

Thanks, I'll try to come up with a reproducer.

nvjle · 2024-02-28T16:45:50Z

CC @nvjle (not sure if notifications are sent without explicit mention)

Thanks @piotrAMD, I'll look into this in detail sometime this week. At first glance it does seem there may be a potential problem, but there is no actual failure that I can see. All testing is good thus far, including for AMDGPU. But we surely want this to be bullet proof.

Thanks, I'll try to come up with a reproducer.

Hi @piotrAMD, I've revisited this and agree with your reasoning and posted this tiny patch accordingly: #83284. All tests (including the backend I'm working on which is at least as SearchableTable-heavy as AMDGPU) are good and WMMAOpcode2AddrMappingTable isn't actually affected one way or the other.

piotrAMD · 2024-02-29T10:31:20Z

Thanks! I tried, but wasn't able to come up with a reproducer.

wangpc-pp requested review from wangpc-pp, Paul-C-Anagnostopoulos, nhaehnle and TNorthover February 4, 2024 02:55

wangpc-pp reviewed Feb 4, 2024

View reviewed changes

Address review comments.

c16b70e

Move the instruction-to-int map into CodeGenTarget::InstrToIntMap and add CodeGenTarget::getInstrIntValue query method. Populate the map in CodeGenTarget::ComputeInstrsByEnum just below where the order is created in the first place.

wangpc-pp approved these changes Feb 5, 2024

View reviewed changes

nvjle added 2 commits February 5, 2024 07:38

Address review nit.

70396ee

Add derived definitions guard.

Address review comments.

1fae470

Use getAllDerivedDefinitionsIfDefined.

wangpc-pp merged commit 8ae0485 into llvm:main Feb 7, 2024

nvjle mentioned this pull request Feb 8, 2024

[TableGen][NFC] Replace hardcoded opcode numbering. #81065

Merged

topperc reviewed Feb 8, 2024

View reviewed changes

nvjle mentioned this pull request Feb 28, 2024

[TableGen] Use consistent field kind checking in getNumericKey. #83284

Merged

	if (Records.getClass("Instruction"))
	if (!Records.getAllDerivedDefinitions("Instruction").empty())

[TableGen] Extend direct lookup to instruction values in generic tables. #80486

[TableGen] Extend direct lookup to instruction values in generic tables. #80486

Uh oh!

Conversation

nvjle commented Feb 2, 2024

Uh oh!

nvjle commented Feb 2, 2024

Uh oh!

wangpc-pp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvjle commented Feb 4, 2024

Uh oh!

wangpc-pp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piotrAMD commented Feb 26, 2024

Uh oh!

piotrAMD commented Feb 27, 2024

Uh oh!

nvjle commented Feb 27, 2024

Uh oh!

piotrAMD commented Feb 27, 2024

Uh oh!

nvjle commented Feb 28, 2024

Uh oh!

piotrAMD commented Feb 29, 2024

Uh oh!

Uh oh!

topperc Feb 8, 2024 •

edited

Loading