Page MenuHomePhabricator

[TableGen] Support encoding and decoding per-HwMode
ClosedPublic

Authored by jmolloy on Sep 13 2019, 3:58 AM.

Details

Summary

Much like ValueTypeByHwMode/RegInfoByHwMode, this patch allows targets
to modify an instruction's encoding based on HwMode. When the
EncodingInfos field is non-empty the Inst and Size fields of the Instruction
are ignored and taken from EncodingInfos instead.

As part of this promote getHwMode() from TargetSubtargetInfo to MCSubtargetInfo.

This new feature is not used by any of the existing in-tree targets - new code
is generated only if targets use EncodingByHwMode.
That being said, there's basic test coverage in llvm/test/TableGen/HwModeEncodeDecode.td

Diff Detail

Repository
rL LLVM

Event Timeline

jmolloy created this revision.Sep 13 2019, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 3:58 AM
lebedev.ri added a subscriber: lebedev.ri.

Can you quote where https://llvm.org/docs/DeveloperPolicy.html#test-cases says it's okay to commit dead code with no test coverage?

Hi Roman,

What are you referring to?

(a) This diff has not been submitted, it is a patch for code review. If it doesn't have tests, that is something that can be mentioned in this review without being passive-aggressive.
(b) This diff has tests.

Cheers,

James

lebedev.ri edited the summary of this revision. (Show Details)Sep 13 2019, 4:26 AM

Hi Roman,

What are you referring to?

(a) This diff has not been submitted, it is a patch for code review.

Yep!

If it doesn't have tests, that is something that can be mentioned in this review without being passive-aggressive.

For future reference, how those my two review comments are passive-aggressive?
From where i stand that remark is.

(b) This diff has tests.

Cool, let's make that more obvious :)
Usually in most other cases that wording "NFC for in-tree targets" means something different..

Cheers,

James

Hi Roman,

Hi Roman,

What are you referring to?

(a) This diff has not been submitted, it is a patch for code review.

Yep!

If it doesn't have tests, that is something that can be mentioned in this review without being passive-aggressive.

For future reference, how those my two review comments are passive-aggressive?
From where i stand that remark is.

Your comment was (parsed by me as being) accusatory of submitting code without test coverage. The comment is written to be deliberately unanswerable as you well know the section you linked to says not to submit code without test coverage.

This use of rhetoric will come across as accusatory to some. In future, to avoid misinterpretation, perhaps phrase more lightly; something like "This doesn't look NFC to me, shouldn't it have tests?".

(b) This diff has tests.

Cool, let's make that more obvious :)
Usually in most other cases that wording "NFC for in-tree targets" means something different..

I'm happy to reword the commit message. Note that "NFC for in-tree targets" implies it's not NFC for non-in-tree targets, but I'm very happy to add clarity to the commit message.

In future, to avoid misinterpretation, it might be a good idea to read the patch fully before accusing a submitter of not adding tests.

Cheers,

James

jmolloy edited reviewers, added: RKSimon, kparzysz; removed: chandlerc, jfb.Sep 15 2019, 1:14 AM

Adding Simon and Krzysztof as the original authors of InfoByHwMode.

RKSimon added inline comments.Mon, Sep 16, 8:33 AM
llvm/utils/TableGen/CodeEmitterGen.cpp
55

Separate NFC clang-format patch?

87

Separate NFC clang-format patch?

263

(style) auto *DI =

265

(style) Don't use auto if the type isn't obvious.

343

(style) Don't use auto if the type isn't obvious.

361

(style) auto *DI =

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp
2398

(style) auto *DI =

2468

Can we move the DecoderNamespace creation inside the populateInstruction case?

jmolloy updated this revision to Diff 220441.Tue, Sep 17, 1:06 AM
jmolloy marked 8 inline comments as done.

Thanks Simon!

kparzysz added inline comments.Tue, Sep 17, 6:09 AM
llvm/utils/TableGen/SubtargetEmitter.cpp
1876

This is probably unnecessary with your change.

1934

This is probably also unnecessary now.

jmolloy marked an inline comment as done.Wed, Sep 18, 1:53 AM

Thanks Krzysztof!

llvm/utils/TableGen/SubtargetEmitter.cpp
1876

Unfortunately it's still necessary, as TargetSubtargetInfo and MCSubtargetInfo are not related. We need HwModes in both (test failures bear this out).

Looks good to me.

llvm/utils/TableGen/SubtargetEmitter.cpp
1876

TargetSubtargetInfo inherits from MCSubtargetInfo, but you're right---each target defines its own subclass of MCSubtargetInfo which is not a part of TargetSubtargetInfo, so both need to be modified.

Don't forget to generate diff with context

llvm/utils/TableGen/CodeEmitterGen.cpp
403

(style) Don't use auto if the type isn't obvious.

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp
2418

(style) Don't use auto if the type isn't obvious.

jmolloy updated this revision to Diff 220697.Wed, Sep 18, 10:04 AM

Thanks Simon! The patch was uploaded with -U999, but this one's with -U9999 just in case :)

Hi Simon,

Are you happy for me to go ahead and commit this?

Cheers,

James

RKSimon accepted this revision.Thu, Sep 19, 3:52 AM

LGTM

This revision is now accepted and ready to land.Thu, Sep 19, 3:52 AM