This is an archive of the discontinued LLVM Phabricator instance.

[Codegen][tablgen][NFC] Allow meta instruction to be target dependent
ClosedPublic

Authored by skan on Mar 14 2022, 6:30 AM.

Details

Summary

An instruction is a meta-instruction if it doesn't produce any output
in the form of executable instructions. So in the concept, a
meta-instruction does not have to be target independent.

Before this patch, isMetaInstruction is implemented by checking the
opcode of the instruction, add we have no way to add target dependent
opcode to the list, which does not make sense.

After this patch, a bit isMeta is added for class Instruction in
tablegen, which is used to indicate whether it's a meta instruction.

Diff Detail

Unit TestsFailed

Event Timeline

skan created this revision.Mar 14 2022, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 6:30 AM
skan requested review of this revision.Mar 14 2022, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 6:30 AM

Is this an alternative of D92842?

llvm/include/llvm/MC/MCInstrDesc.h
268

Duplicated doesn't.

skan added a comment.Mar 14 2022, 7:32 AM

Is this an alternative of D92842?

No, these two patches have different purposes. This is to allow meta instruction to be target dependent, but D121600 is to unify the MEMBARRIER for different targets. But they can fix the same bug, which is a side effect.

skan updated this revision to Diff 415090.Mar 14 2022, 7:35 AM
skan marked an inline comment as done.

Fix typo in comments

skan retitled this revision from [Codegen] Allow meta instruction to be target dependent to [Codegen][X86] Allow meta instruction to be target dependent.Mar 16 2022, 12:01 AM
skan retitled this revision from [Codegen][X86] Allow meta instruction to be target dependent to [Codegen][X86][XCore] Allow meta instruction to be target dependent.

If you’re fixing a bug shouldn’t there be a test?

skan added a comment.Mar 16 2022, 7:01 PM

If you’re fixing a bug shouldn’t there be a test?

We have a unit test in MachineMetadata.cpp.

If you’re fixing a bug shouldn’t there be a test?

We have a unit test in MachineMetadata.cpp.

Is that test doing anything more than checking that isMetaInstruction returns true for the test?

skan added a comment.EditedMar 16 2022, 7:29 PM

If you’re fixing a bug shouldn’t there be a test?

We have a unit test in MachineMetadata.cpp.

Is that test doing anything more than checking that isMetaInstruction returns true for the test?

It only checks isMetaInstruction returns true for Int_MemBarrier. There could be lots of bugs if isMetaInstruction returns false for an instruction that emits nothing executable. One of them is that the redundant dwarf debug location could be generated (llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp).
And unit test is the simplest way to check.

Maybe break this into 2 patches. One for adding the target independent bit and one for bug fix?

skan updated this revision to Diff 416066.Mar 16 2022, 9:35 PM

Split this patch: One for adding the target independent bit and one for bug fix.

skan edited the summary of this revision. (Show Details)Mar 16 2022, 9:43 PM
skan retitled this revision from [Codegen][X86][XCore] Allow meta instruction to be target dependent to [Codegen][tablgen][NFC] Allow meta instruction to be target dependent.
pengfei accepted this revision.Mar 16 2022, 10:03 PM

LGTM, but wait one or two days for other reviewers' opinions.

This revision is now accepted and ready to land.Mar 16 2022, 10:03 PM
skan added a comment.Mar 16 2022, 10:37 PM

LGTM, but wait one or two days for other reviewers' opinions.

Thanks! A LIT test is provided at D121879.

This revision was landed with ongoing or failed builds.Mar 17 2022, 10:09 PM
This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Mar 22 2022, 4:14 AM
bjope added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
815

@skan (and reviewerts): Did you consider what the default QueryType should be here?

Downstream we've been using IgnoreBundle for isMetaInstruction (similarly as for isPseudo above). And now a number of debug info related test cases started to fail.

Before your patch isMetaInstruction() returned false when being queried on a BUNDLE instruction. So I think it would be more safe to use IgnoreBundle unless you want to change that behavior. And I don't think usage of isMetaInstruction in for example llvm::calculateDbgEntityHistory is supposed to skip a bundle just because one of multiple instructions in a BUNDLE is a meta instruction.

skan added inline comments.Mar 22 2022, 4:57 AM
llvm/include/llvm/CodeGen/MachineInstr.h
815

Thanks for the information! I prepared a patch here D122221.