Get rid of the "; mem:" suffix and use the one we use in MIR: ":: (load 2)".
Details
Diff Detail
Event Timeline
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. |
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. |
- 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
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. |
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. |
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. |
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. |
Should this be SmallVectorImpl<StringRef> rather than forcing the size? Maybe even an ArrayRef?