[DebugInfo][InstrRef] Treat ORRWrr as a copy instr #123102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rastogishubham
requested review from
jmorse,
adrian-prantl and
felipepiovezan
|
@llvm/pr-subscribers-backend-aarch64 Author: Shubham Sandeep Rastogi (rastogishubham) ChangesThe insturction selector uses the MachineFunction::copySalvageSSA function to insert DBG_PHIs or identify a defining instruction for a copy-like instruction when finalizing Instruction References. AArch64 has the ORR instruction which is a logical OR with the variants ORRWrr which refers to a register to register variant, and ORRWrs which is a register to a shifted register variant. An ORRWrs where the shift amount is 0, and the zero register ($wzr) is used is considered a copy, for example: $w0 = ORRWrs $wzr, killed $w3, 0 however an ORRWrr with a zero register is not considered a copy $w0 = ORRWr $wzr, killed $w3 This causes an issue in the livedebugvalues pass because in aarch64-isel the instruction is the ORRWrr variant, but is then changed to the ORRWrs variant before the livedebugvalues pass. This causes a mismatch between the two passes which leads to a crash in the livedebugvalues pass. This patch fixes the issue. Full diff: https://github.com/llvm/llvm-project/pull/123102.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 8a3ed10b8e0bd2..e3f60f6cd7253c 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9722,9 +9722,11 @@ AArch64InstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
// AArch64::ORRWrs and AArch64::ORRXrs with WZR/XZR reg
// and zero immediate operands used as an alias for mov instruction.
- if (MI.getOpcode() == AArch64::ORRWrs &&
- MI.getOperand(1).getReg() == AArch64::WZR &&
- MI.getOperand(3).getImm() == 0x0 &&
+ if (((MI.getOpcode() == AArch64::ORRWrs &&
+ MI.getOperand(1).getReg() == AArch64::WZR &&
+ MI.getOperand(3).getImm() == 0x0) ||
+ (MI.getOpcode() == AArch64::ORRWrr &&
+ MI.getOperand(1).getReg() == AArch64::WZR)) &&
// Check that the w->w move is not a zero-extending w->x mov.
(!MI.getOperand(0).getReg().isVirtual() ||
MI.getOperand(0).getSubReg() == 0) &&
@@ -9744,9 +9746,11 @@ AArch64InstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
std::optional<DestSourcePair>
AArch64InstrInfo::isCopyLikeInstrImpl(const MachineInstr &MI) const {
- if (MI.getOpcode() == AArch64::ORRWrs &&
- MI.getOperand(1).getReg() == AArch64::WZR &&
- MI.getOperand(3).getImm() == 0x0)
+ if ((MI.getOpcode() == AArch64::ORRWrs &&
+ MI.getOperand(1).getReg() == AArch64::WZR &&
+ MI.getOperand(3).getImm() == 0x0) ||
+ (MI.getOpcode() == AArch64::ORRWrr &&
+ MI.getOperand(1).getReg() == AArch64::WZR))
return DestSourcePair{MI.getOperand(0), MI.getOperand(2)};
return std::nullopt;
}
diff --git a/llvm/test/CodeGen/AArch64/instr-ref-ldv.ll b/llvm/test/CodeGen/AArch64/instr-ref-ldv.ll
new file mode 100644
index 00000000000000..7c40a47ee58348
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/instr-ref-ldv.ll
@@ -0,0 +1,84 @@
+; RUN: llc -O2 -experimental-debug-variable-locations %s -stop-after=livedebugvalues -mtriple=arm64-apple-macosx15.0.0 -o - | FileCheck %s
+
+; CHECK: $w0 = ORRWrs $wzr, killed $w3, 0
+; CHECK-NEXT: DBG_INSTR_REF !7, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(3, 0), debug-location !11
+
+
+%"class.llvm::iterator_range.53" = type { %"class.llvm::opt::arg_iterator.54", %"class.llvm::opt::arg_iterator.54" }
+%"class.llvm::opt::arg_iterator.54" = type { %"class.std::__1::reverse_iterator", %"class.std::__1::reverse_iterator", [2 x %"class.llvm::opt::OptSpecifier"] }
+%"class.std::__1::reverse_iterator" = type { ptr, ptr }
+%"class.llvm::opt::OptSpecifier" = type { i32 }
+declare noundef zeroext i1 @_ZNK4llvm3opt6Option7matchesENS0_12OptSpecifierE(ptr noundef nonnull align 8 dereferenceable(16), i64) local_unnamed_addr #1
+define noundef zeroext i1 @_ZNK4llvm3opt7ArgList14hasFlagNoClaimENS0_12OptSpecifierES2_b(ptr noundef nonnull align 8 dereferenceable(184) %this, i64 %Pos.coerce, i64 %Neg.coerce, i1 noundef zeroext %Default) local_unnamed_addr #2 !dbg !9383 {
+entry:
+ %ref.tmp.i = alloca %"class.llvm::iterator_range.53", align 8
+ %coerce.val.ii6 = and i64 %Pos.coerce, 4294967295, !dbg !9393
+ #dbg_value(i64 %coerce.val.ii6, !9452, !DIExpression(), !9480)
+ %__begin0.sroa.4.0.ref.tmp.sroa_idx.i = getelementptr inbounds i8, ptr %ref.tmp.i, i64 8, !dbg !9489
+ %__begin0.sroa.4.0.copyload.i = load ptr, ptr %__begin0.sroa.4.0.ref.tmp.sroa_idx.i, align 8, !dbg !9489
+ %__end0.sroa.4.0.end_iterator.i.sroa_idx.i = getelementptr inbounds i8, ptr %ref.tmp.i, i64 48, !dbg !9495
+ %__end0.sroa.4.0.copyload.i = load ptr, ptr %__end0.sroa.4.0.end_iterator.i.sroa_idx.i, align 8, !dbg !9495
+ %cmp.i.i.i.not.i = icmp eq ptr %__begin0.sroa.4.0.copyload.i, %__end0.sroa.4.0.copyload.i, !dbg !9522
+ br i1 %cmp.i.i.i.not.i, label %_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit.thread, label %_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit, !dbg !9523
+_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit.thread: ; preds = %entry
+ br label %1, !dbg !9525
+_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit: ; preds = %entry
+ %incdec.ptr.i.i.i = getelementptr inbounds i8, ptr %__begin0.sroa.4.0.copyload.i, i64 -8, !dbg !9526
+ %0 = load ptr, ptr %incdec.ptr.i.i.i, align 8, !dbg !9527, !tbaa !9528
+ %tobool.not.not = icmp eq ptr %0, null, !dbg !9525
+ br i1 %tobool.not.not, label %1, label %cleanup, !dbg !9525
+cleanup: ; preds = %_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit
+ %call13 = call noundef zeroext i1 @_ZNK4llvm3opt6Option7matchesENS0_12OptSpecifierE(ptr noundef nonnull align 8 dereferenceable(16) %0, i64 %coerce.val.ii6) #3, !dbg !9533
+ br label %1
+ %2 = phi i1 [ %call13, %cleanup ], [ %Default, %_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit ], [ %Default, %_ZNK4llvm3opt7ArgList17getLastArgNoClaimIJNS0_12OptSpecifierES3_EEEPNS0_3ArgEDpT_.exit.thread ]
+ ret i1 %2, !dbg !9534
+}
+!llvm.module.flags = !{ !2, !6}
+!llvm.dbg.cu = !{!7}
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = !{i32 7, !"frame-pointer", i32 1}
+!7 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !8, emissionKind: FullDebug, sdk: "MacOSX15.3.sdk")
+!8 = !DIFile(filename: "/Users/shubhamrastogi/Development/llvm-project-instr-ref/llvm-project/llvm/lib/Option/ArgList.cpp", directory: "/Users/shubhamrastogi/Development/llvm-project-instr-ref/llvm-project/build-instr-ref-stage2", checksumkind: CSK_MD5, checksum: "a3198e8ace679c7b1581a26b5583c658")
+!3116 = distinct !DICompositeType(tag: DW_TAG_class_type, size: 32)
+!9383 = distinct !DISubprogram( unit: !7, flags: DIFlagArtificial | DIFlagObjectPointer)
+!9391 = distinct !DILexicalBlock(scope: !9383, line: 80, column: 12)
+!9393 = !DILocation( scope: !9391)
+!9395 = distinct !DISubprogram( unit: !7, retainedNodes: !9396)
+!9396 = !{}
+!9400 = distinct !DILocation( scope: !9401, inlinedAt: !9439)
+!9401 = distinct !DISubprogram( unit: !7, retainedNodes: !9436)
+!9436 = !{}
+!9439 = distinct !DILocation( scope: !9440, inlinedAt: !9478)
+!9440 = distinct !DILexicalBlock(scope: !9441, line: 269, column: 5)
+!9441 = distinct !DILexicalBlock(scope: !9442, line: 269, column: 5)
+!9442 = distinct !DISubprogram( unit: !7, retainedNodes: !9450)
+!9450 = !{}
+!9452 = !DILocalVariable( scope: !9442, type: !3116)
+!9478 = distinct !DILocation( scope: !9391)
+!9480 = !DILocation( scope: !9441, inlinedAt: !9478)
+!9484 = distinct !DISubprogram( unit: !7, retainedNodes: !9485)
+!9485 = !{}
+!9488 = distinct !DILocation( scope: !9441, inlinedAt: !9478)
+!9489 = !DILocation( scope: !9484, inlinedAt: !9488)
+!9491 = distinct !DISubprogram( unit: !7, retainedNodes: !9492)
+!9492 = !{}
+!9494 = distinct !DILocation( scope: !9441, inlinedAt: !9478)
+!9495 = !DILocation( scope: !9491, inlinedAt: !9494)
+!9497 = distinct !DISubprogram( unit: !7, retainedNodes: !9500)
+!9500 = !{}
+!9503 = distinct !DILocation( scope: !9441, inlinedAt: !9478)
+!9505 = distinct !DISubprogram( unit: !7, retainedNodes: !9509)
+!9509 = !{}
+!9515 = distinct !DILocation( scope: !9516, inlinedAt: !9520)
+!9516 = distinct !DISubprogram( unit: !7, retainedNodes: !9517)
+!9517 = !{}
+!9520 = distinct !DILocation( scope: !9497, inlinedAt: !9503)
+!9522 = !DILocation( scope: !9505, inlinedAt: !9515)
+!9523 = !DILocation( scope: !9441, inlinedAt: !9478)
+!9525 = !DILocation( scope: !9391)
+!9526 = !DILocation( scope: !9395, inlinedAt: !9400)
+!9527 = !DILocation( scope: !9440, inlinedAt: !9478)
+!9528 = !{!"any pointer", !9530, i64 0}
+!9530 = !{}
+!9533 = !DILocation( scope: !9391)
+!9534 = !DILocation( scope: !9383)
|
rastogishubham
force-pushed
the
FixLDVAarch64
branch
from
8012052 to
8849b71
Compare
rastogishubham
force-pushed
the
FixLDVAarch64
branch
from
8849b71 to
5d2206f
Compare
jmorse
approved these changes
Jan 16, 2025
There was a problem hiding this comment.
LGTM; this is really interesting in that copy-like instructions mutate between being unrecognisable and then recognisable as copies. I wonder if in the long term this might force too much logic into isCopyInstrImpl, but this fix to it is good.
I feel the urge to say "this should be an MIR test", but I suppose this is checking how isel produces a copy and how it mutates over the compilation, and that it doesn't crash. So, the test is probably fine as it is.
I think yes -- there are pros and cons of each approach, but ultimately we should be consistent between the two passes. |
adrian-prantl
approved these changes
Jan 16, 2025
The insturction selector uses the MachineFunction::copySalvageSSA function to insert DBG_PHIs or identify a defining instruction for a copy-like instruction when finalizing Instruction References. AArch64 has the ORR instruction which is a logical OR with the variants ORRWrr which refers to a register to register variant, and ORRWrs which is a register to a shifted register variant. An ORRWrs where the shift amount is 0, and the zero register ($wzr) is used is considered a copy, for example: $w0 = ORRWrs $wzr, killed $w3, 0 however an ORRWrr with a zero register is not considered a copy $w0 = ORRWr $wzr, killed $w3 This causes an issue in the livedebugvalues pass because in aarch64-isel the instruction is the ORRWrr variant, but is then changed to the ORRWrs variant before the livedebugvalues pass. This causes a mismatch between the two passes which leads to a crash in the livedebugvalues pass. This patch fixes the issue.
rastogishubham
force-pushed
the
FixLDVAarch64
branch
from
5d2206f to
5851cfe
Compare
Yes the only reason it isn't a MIR test is because we need to make sure that isel produces the copy and then how it mutates over the passes. If we just replaced it with mir, then the test wouldn't crash if this patch wasn't applied. Thanks! |
rastogishubham
added a commit
to rastogishubham/llvm-project
that referenced
this pull request
Jan 25, 2025
The insturction selector uses the `MachineFunction::copySalvageSSA` function to insert `DBG_PHIs` or identify a defining instruction for a copy-like instruction when finalizing Instruction References. AArch64 has the ORR instruction which is a logical OR with the variants ORRWrr which refers to a register to register variant, and ORRWrs which is a register to a shifted register variant. An ORRWrs where the shift amount is 0, and the zero register ($wzr) is used is considered a copy, for example: `$w0 = ORRWrs $wzr, killed $w3, 0` However an ORRWrr with a zero register is not considered a copy `$w0 = ORRWrr $wzr, killed $w3` This causes an issue in the livedebugvalues pass because in aarch64-isel the instruction is the ORRWrr variant, but is then changed to the ORRWrs variant before the livedebugvalues pass. This causes a mismatch between the two passes which leads to a crash in the livedebugvalues pass. This patch fixes the issue. (cherry picked from commit ee1c852)
adrian-prantl
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jan 29, 2025
[DebugInfo][InstrRef] Treat ORRWrr as a copy instr (llvm#123102)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The insturction selector uses the
MachineFunction::copySalvageSSAfunction to insertDBG_PHIsor identify a defining instruction for a copy-like instruction when finalizing Instruction References.AArch64 has the ORR instruction which is a logical OR with the variants ORRWrr which refers to a register to register variant, and ORRWrs which is a register to a shifted register variant.
An ORRWrs where the shift amount is 0, and the zero register ($wzr) is used is considered a copy, for example:
$w0 = ORRWrs $wzr, killed $w3, 0However an ORRWrr with a zero register is not considered a copy
$w0 = ORRWrr $wzr, killed $w3This causes an issue in the livedebugvalues pass because in aarch64-isel the instruction is the ORRWrr variant, but is then changed to the ORRWrs variant before the livedebugvalues pass.
This causes a mismatch between the two passes which leads to a crash in the livedebugvalues pass.
This patch fixes the issue.