This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Use MIR syntax for MachineMemOperand printing
ClosedPublic

Authored by thegameg on Jan 22 2018, 8:09 AM.

Diff Detail

Event Timeline

thegameg created this revision.Jan 22 2018, 8:09 AM
thegameg added inline comments.Jan 23 2018, 8:45 AM
lib/CodeGen/MachineOperand.cpp
992

I just noticed this was printed before and won't be printed anymore after this patch.

IIUC, here we print the alignment of the base if it is affected by the offset, while in MIR and in this file at the line 999 we only care about the base alignment and only print the alignment of the whole reference.

Please correct me if I'm wrong.

thegameg added inline comments.Jan 23 2018, 9:32 AM
lib/CodeGen/MachineOperand.cpp
992

The only test in this case is CodeGen/AMDGPU/extload-align.ll.

The original is:

BUFFER_STORE_SHORT_OFFEN %13, %stack.0, %sgpr96_sgpr97_sgpr98_sgpr99, %sgpr3, 2, 0, 0, 0, implicit %exec; mem:ST2[FixedStack0(addrspace=5)(align=8)+2](align=2)

after this patch:

BUFFER_STORE_SHORT_OFFEN %13, %stack.0, %sgpr96_sgpr97_sgpr98_sgpr99, %sgpr3, 2, 0, 0, 0, implicit %exec :: (store 2 into %stack.0 + 2, align 8, addrspace 5)

The MIR parser expects that alignment to be the base alignment, while when reading it it looks like this is the alignment of the whole reference.

thegameg updated this revision to Diff 131906.Jan 29 2018, 5:36 PM
thegameg edited the summary of this revision. (Show Details)

Ping?

  • Added support for addrspace in r323521.
  • Rebase

Ping? This is one of the latest changes for MIR / -debug unification.

nhaehnle removed a subscriber: nhaehnle.Mar 5 2018, 2:59 AM
thegameg updated this revision to Diff 138205.Mar 13 2018, 8:53 AM
  • Rebase
  • Fix new tests:
    • test/CodeGen/AArch64/arm64-misched-memdep-bug.ll
    • test/CodeGen/ARM/misched-int-basic-thumb2.mir
    • test/CodeGen/ARM/single-issue-r52.mir
    • test/CodeGen/Hexagon/post-inc-aa-metadata.ll
    • test/CodeGen/PowerPC/byval-agg-info.ll
bogner added inline comments.Mar 13 2018, 3:32 PM
include/llvm/CodeGen/MachineMemOperand.h
299

Should this be SmallVectorImpl<StringRef> rather than forcing the size? Maybe even an ArrayRef?

lib/CodeGen/MachineInstr.cpp
1438–1448

This is just a dummy vector for the API right? We should be able to make it take less space. Though if we make the argument an ArrayRef we can just pass in None and save the variable entirely.

1445–1446

Would it be clearer to just use "else"?

lib/CodeGen/MachineOperand.cpp
469–480

I'd probably skip the extraneous curly braces here, since you aren't declaring any variables in these cases.

770

Is this an unrelated bug fix that snuck in or something? It isn't obvious to me why it's changing.

thegameg updated this revision to Diff 138408.Mar 14 2018, 10:51 AM
thegameg marked 5 inline comments as done.

Thanks for the review Justin!

include/llvm/CodeGen/MachineMemOperand.h
299

SmallVectorImpl<StringRef> is better, yes. I wish I could make it an ArrayRef but printSyncScope is caching the results from LLVMContext::getSyncScopeNames in SSNs, so we can't use ArrayRef here.

lib/CodeGen/MachineInstr.cpp
1438–1448

I made it <StringRef, 0>, my guess is that most of the time we will have SyncScope::System and won't print anything, but honestly I don't know much about it.

lib/CodeGen/MachineOperand.cpp
770

My bad, this shouldn't be here.

This revision is now accepted and ready to land.Mar 14 2018, 2:10 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.
llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
428 ↗(On Diff #138456)

G isn't guaranteed non-null here. There's a version of SDNode::dump() that passes nullptr. This is most often used when debugging in gdb/lldb. Though I found a use in the X86 backend that I fixed in r327744.

thegameg added inline comments.Mar 18 2018, 1:57 PM
llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
428 ↗(On Diff #138456)

Sorry about that. Put up a patch to fix this: https://reviews.llvm.org/D44611.