This is an archive of the discontinued LLVM Phabricator instance.

[MIR] Add comments to INLINEASM immediate flag MachineOperands
ClosedPublic

Authored by kschwarz on Apr 14 2020, 12:51 AM.

Details

Summary

The INLINEASM MIR instructions use immediate operands to encode the values of some operands.
The MachineInstr pretty printer function already handles those operands and prints human readable annotations instead of the immediates. This patch adds similar annotations to the output of the MIRPrinter, however uses the new MIROperandComment feature.

Diff Detail

Event Timeline

kschwarz created this revision.Apr 14 2020, 12:51 AM

I've thought about adding enum to string conversion functions to InlineAsm.h, which could be reused from both MachineInstr.cpp and TargetInstrInfo.cpp to avoid code duplication.
Would that be preferred here?

kschwarz edited reviewers, added: efriedma; removed: eli.friedman.Apr 14 2020, 12:56 AM

I like it!
Some minor comments and nits in line.

llvm/lib/CodeGen/TargetInstrInfo.cpp
1330

Can we do an early exit here:

if (!MI.isInlineAsm())
  return std::string();

that also reduces some identation below..

1332

do we need OS here?

1338

can we then just do

return "sideeffect";

here, and similar for the if-stmts below?

1353

We don't need the 'else'?

1369

probably just an assert here is better?

1404

and same here?

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
498–508

This comment is out of date now and needs a minor update.

kschwarz marked 2 inline comments as done.Apr 14 2020, 3:07 AM
kschwarz added inline comments.
llvm/lib/CodeGen/TargetInstrInfo.cpp
1332

We could simply append to a std::string here, but the cases below need to append other things (integers), so I felt like using an ostream might be more elegant?

1338

Unfortunately no, the immediate we are looking at here encodes multiple values at once, so we need to compose the string here.

arsenm added inline comments.Apr 14 2020, 7:11 AM
llvm/lib/CodeGen/TargetInstrInfo.cpp
1330

return ""?

1334

This is essentially repeated in MachineInstr, should unify these? I guess we are already duplicated the flag handling

kschwarz updated this revision to Diff 257683.Apr 15 2020, 5:39 AM

Address review comments

kschwarz marked 7 inline comments as done.Apr 15 2020, 5:39 AM
arsenm added inline comments.Apr 15 2020, 6:26 AM
llvm/include/llvm/IR/InlineAsm.h
402–403

llvm_unreachable

450–451

llvm_unreachable

kschwarz updated this revision to Diff 257729.Apr 15 2020, 8:18 AM

Change assert to llvm_unreachable

kschwarz marked 2 inline comments as done.Apr 15 2020, 8:18 AM
arsenm accepted this revision.Apr 15 2020, 10:37 AM
arsenm added inline comments.
llvm/lib/CodeGen/TargetInstrInfo.cpp
1326

Should this write to a raw_ostream rather than returning a string?

This revision is now accepted and ready to land.Apr 15 2020, 10:37 AM
kschwarz marked an inline comment as done.Apr 16 2020, 4:58 AM
kschwarz added inline comments.
llvm/lib/CodeGen/TargetInstrInfo.cpp
1326

I'm not sure why it originally returned a string, but the function already existed before this patch

This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.
llvm/test/CodeGen/X86/stack-folding-fp-nofpexcept.mir
41

So if the register class encodings in the tablegen generated files have changed since the test was created, the input MIR is incorrect and the output will just show whatever class has that encoding today? And will suddently fail the next time the encodings change?

kschwarz added inline comments.Apr 20 2020, 3:23 AM
llvm/test/CodeGen/X86/stack-folding-fp-nofpexcept.mir
41

Yes, the register class is encoded in the "flags" immediate operand (7405578 in this case). If that encoding changes, this test needs to be updated. The change made here simply adds a human-readable comment to the immediate operands. The intent is to make tests more meaningful for humans to read.

skan added a subscriber: skan.Sep 28 2023, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2023, 8:25 AM
Herald added a subscriber: pengfei. · View Herald Transcript