Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GlobalISel] Adding support for handling G_ASSERT_{SEXT,ZEXT,ALIGN} i… #74196

Merged

Conversation

dfszabo
Copy link
Contributor

@dfszabo dfszabo commented Dec 2, 2023

…n artifact combiner

These instructions are hint for optimizations and can be treated as copies and are handled as such with this change. Without it is possible to run into an assertion, since tryCombineUnmergeValues rightfully use getDefIgnoringCopies to get the source MI, which already handle these hint instructions and treat them as copies. The problem is that markDefDead only considers COPYs, which will lead to crash with assertion for cases like testUnmergeHintOfTrunc.

…n artifact combiner

These instructions are hint for optimizations and can be treated as copies and are handled as such with this change. Without it is possible to run into an assertion, since tryCombineUnmergeValues rightfully use getDefIgnoringCopies to get the source MI, which already handle these hint instructions and treat them as copies. The problem is that markDefDead only considers COPYs, which will lead to crash with assertion for cases like testUnmergeHintOfTrunc.
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 2, 2023

@llvm/pr-subscribers-llvm-globalisel

Author: Dávid Ferenc Szabó (dfszabo)

Changes

…n artifact combiner

These instructions are hint for optimizations and can be treated as copies and are handled as such with this change. Without it is possible to run into an assertion, since tryCombineUnmergeValues rightfully use getDefIgnoringCopies to get the source MI, which already handle these hint instructions and treat them as copies. The problem is that markDefDead only considers COPYs, which will lead to crash with assertion for cases like testUnmergeHintOfTrunc.


Full diff: https://github.com/llvm/llvm-project/pull/74196.diff

3 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h (+10-11)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll (+1-4)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir (+64)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
index 851353042cc26..da330b517c280 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
@@ -1366,6 +1366,9 @@ class LegalizationArtifactCombiner {
           // Adding Use to ArtifactList.
           WrapperObserver.changedInstr(Use);
           break;
+        case TargetOpcode::G_ASSERT_SEXT:
+        case TargetOpcode::G_ASSERT_ZEXT:
+        case TargetOpcode::G_ASSERT_ALIGN:
         case TargetOpcode::COPY: {
           Register Copy = Use.getOperand(0).getReg();
           if (Copy.isVirtual())
@@ -1392,6 +1395,9 @@ class LegalizationArtifactCombiner {
     case TargetOpcode::G_ANYEXT:
     case TargetOpcode::G_SEXT:
     case TargetOpcode::G_EXTRACT:
+    case TargetOpcode::G_ASSERT_SEXT:
+    case TargetOpcode::G_ASSERT_ZEXT:
+    case TargetOpcode::G_ASSERT_ALIGN:
       return MI.getOperand(1).getReg();
     case TargetOpcode::G_UNMERGE_VALUES:
       return MI.getOperand(MI.getNumOperands() - 1).getReg();
@@ -1425,7 +1431,8 @@ class LegalizationArtifactCombiner {
       if (MRI.hasOneUse(PrevRegSrc)) {
         if (TmpDef != &DefMI) {
           assert((TmpDef->getOpcode() == TargetOpcode::COPY ||
-                  isArtifactCast(TmpDef->getOpcode())) &&
+                  isArtifactCast(TmpDef->getOpcode()) ||
+                  isPreISelGenericOptimizationHint(TmpDef->getOpcode())) &&
                  "Expecting copy or artifact cast here");
 
           DeadInsts.push_back(TmpDef);
@@ -1509,16 +1516,8 @@ class LegalizationArtifactCombiner {
   /// Looks through copy instructions and returns the actual
   /// source register.
   Register lookThroughCopyInstrs(Register Reg) {
-    using namespace llvm::MIPatternMatch;
-
-    Register TmpReg;
-    while (mi_match(Reg, MRI, m_Copy(m_Reg(TmpReg)))) {
-      if (MRI.getType(TmpReg).isValid())
-        Reg = TmpReg;
-      else
-        break;
-    }
-    return Reg;
+    Register TmpReg = getSrcRegIgnoringCopies(Reg, MRI);
+    return TmpReg.isValid() ? TmpReg : Reg;
   }
 };
 
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll b/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll
index a7d7a1e81617e..5867326c18aa6 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll
@@ -46,14 +46,11 @@ define i32 @test(i32 %a, i1 %c) {
   ; PRESELECTION-NEXT: {{  $}}
   ; PRESELECTION-NEXT:   [[COPY:%[0-9]+]]:gpr(s32) = COPY $w0
   ; PRESELECTION-NEXT:   [[COPY1:%[0-9]+]]:gpr(s32) = COPY $w1
-  ; PRESELECTION-NEXT:   [[TRUNC:%[0-9]+]]:gpr(s8) = G_TRUNC [[COPY1]](s32)
-  ; PRESELECTION-NEXT:   [[ASSERT_ZEXT:%[0-9]+]]:gpr(s8) = G_ASSERT_ZEXT [[TRUNC]], 1
   ; PRESELECTION-NEXT:   [[C:%[0-9]+]]:gpr(s32) = G_CONSTANT i32 0
   ; PRESELECTION-NEXT:   [[C1:%[0-9]+]]:gpr(s32) = G_CONSTANT i32 100000
   ; PRESELECTION-NEXT:   [[CONSTANT_FOLD_BARRIER:%[0-9]+]]:gpr(s32) = G_CONSTANT_FOLD_BARRIER [[C1]]
-  ; PRESELECTION-NEXT:   [[ANYEXT:%[0-9]+]]:gpr(s32) = G_ANYEXT [[ASSERT_ZEXT]](s8)
   ; PRESELECTION-NEXT:   [[C2:%[0-9]+]]:gpr(s32) = G_CONSTANT i32 1
-  ; PRESELECTION-NEXT:   [[AND:%[0-9]+]]:gpr(s32) = G_AND [[ANYEXT]], [[C2]]
+  ; PRESELECTION-NEXT:   [[AND:%[0-9]+]]:gpr(s32) = G_AND [[COPY1]], [[C2]]
   ; PRESELECTION-NEXT:   G_BRCOND [[AND]](s32), %bb.3
   ; PRESELECTION-NEXT:   G_BR %bb.2
   ; PRESELECTION-NEXT: {{  $}}
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir b/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir
index 62865bcfdf081..ce02aa380efe1 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir
@@ -136,6 +136,47 @@ body:             |
     $x0 = COPY %3(s64)
     RET_ReallyLR implicit $x0
 
+...
+---
+name:            testExtOfHintOfTrunc
+body:             |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: testExtOfHintOfTrunc
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: $x0 = COPY [[COPY]](s64)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s8) = G_TRUNC %0(s64)
+    %2:_(s8) = G_ASSERT_ZEXT %1(s8), 1
+    %3:_(s64) = G_ANYEXT %2(s8)
+    $x0 = COPY %3(s64)
+    RET_ReallyLR implicit $x0
+
+...
+---
+name:            testExtOfCopyAndHintOfTrunc
+body:             |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: testExtOfCopyAndHintOfTrunc
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: $x0 = COPY [[COPY]](s64)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s8) = G_TRUNC %0(s64)
+    %2:_(s8) = COPY %1(s8)
+    %3:_(s8) = G_ASSERT_ZEXT %2(s8), 1
+    %4:_(s64) = G_ANYEXT %3(s8)
+    $x0 = COPY %4(s64)
+    RET_ReallyLR implicit $x0
+
 ...
 ---
 name:            testExtOf2CopyOfTrunc
@@ -158,3 +199,26 @@ body:             |
     RET_ReallyLR implicit $x0
 
 ...
+---
+name:            testUnmergeHintOfTrunc
+body:             |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: testUnmergeHintOfTrunc
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: [[UV:%[0-9]+]]:_(s8), [[UV1:%[0-9]+]]:_(s8), [[UV2:%[0-9]+]]:_(s8), [[UV3:%[0-9]+]]:_(s8), [[UV4:%[0-9]+]]:_(s8), [[UV5:%[0-9]+]]:_(s8), [[UV6:%[0-9]+]]:_(s8), [[UV7:%[0-9]+]]:_(s8) = G_UNMERGE_VALUES [[COPY]](s64)
+    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[UV1]](s8)
+    ; CHECK-NEXT: $x0 = COPY [[ANYEXT]](s64)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s16) = G_TRUNC %0(s64)
+    %2:_(s16) = G_ASSERT_ZEXT %1(s16), 1
+    %3:_(s8), %4:_(s8) = G_UNMERGE_VALUES %2(s16)
+    %5:_(s64) = G_ANYEXT %4(s8)
+    $x0 = COPY %5(s64)
+    RET_ReallyLR implicit $x0
+
+...

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 2, 2023

@llvm/pr-subscribers-backend-aarch64

Author: Dávid Ferenc Szabó (dfszabo)

Changes

…n artifact combiner

These instructions are hint for optimizations and can be treated as copies and are handled as such with this change. Without it is possible to run into an assertion, since tryCombineUnmergeValues rightfully use getDefIgnoringCopies to get the source MI, which already handle these hint instructions and treat them as copies. The problem is that markDefDead only considers COPYs, which will lead to crash with assertion for cases like testUnmergeHintOfTrunc.


Full diff: https://github.com/llvm/llvm-project/pull/74196.diff

3 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h (+10-11)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll (+1-4)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir (+64)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
index 851353042cc26..da330b517c280 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
@@ -1366,6 +1366,9 @@ class LegalizationArtifactCombiner {
           // Adding Use to ArtifactList.
           WrapperObserver.changedInstr(Use);
           break;
+        case TargetOpcode::G_ASSERT_SEXT:
+        case TargetOpcode::G_ASSERT_ZEXT:
+        case TargetOpcode::G_ASSERT_ALIGN:
         case TargetOpcode::COPY: {
           Register Copy = Use.getOperand(0).getReg();
           if (Copy.isVirtual())
@@ -1392,6 +1395,9 @@ class LegalizationArtifactCombiner {
     case TargetOpcode::G_ANYEXT:
     case TargetOpcode::G_SEXT:
     case TargetOpcode::G_EXTRACT:
+    case TargetOpcode::G_ASSERT_SEXT:
+    case TargetOpcode::G_ASSERT_ZEXT:
+    case TargetOpcode::G_ASSERT_ALIGN:
       return MI.getOperand(1).getReg();
     case TargetOpcode::G_UNMERGE_VALUES:
       return MI.getOperand(MI.getNumOperands() - 1).getReg();
@@ -1425,7 +1431,8 @@ class LegalizationArtifactCombiner {
       if (MRI.hasOneUse(PrevRegSrc)) {
         if (TmpDef != &DefMI) {
           assert((TmpDef->getOpcode() == TargetOpcode::COPY ||
-                  isArtifactCast(TmpDef->getOpcode())) &&
+                  isArtifactCast(TmpDef->getOpcode()) ||
+                  isPreISelGenericOptimizationHint(TmpDef->getOpcode())) &&
                  "Expecting copy or artifact cast here");
 
           DeadInsts.push_back(TmpDef);
@@ -1509,16 +1516,8 @@ class LegalizationArtifactCombiner {
   /// Looks through copy instructions and returns the actual
   /// source register.
   Register lookThroughCopyInstrs(Register Reg) {
-    using namespace llvm::MIPatternMatch;
-
-    Register TmpReg;
-    while (mi_match(Reg, MRI, m_Copy(m_Reg(TmpReg)))) {
-      if (MRI.getType(TmpReg).isValid())
-        Reg = TmpReg;
-      else
-        break;
-    }
-    return Reg;
+    Register TmpReg = getSrcRegIgnoringCopies(Reg, MRI);
+    return TmpReg.isValid() ? TmpReg : Reg;
   }
 };
 
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll b/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll
index a7d7a1e81617e..5867326c18aa6 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-hoisted-constants.ll
@@ -46,14 +46,11 @@ define i32 @test(i32 %a, i1 %c) {
   ; PRESELECTION-NEXT: {{  $}}
   ; PRESELECTION-NEXT:   [[COPY:%[0-9]+]]:gpr(s32) = COPY $w0
   ; PRESELECTION-NEXT:   [[COPY1:%[0-9]+]]:gpr(s32) = COPY $w1
-  ; PRESELECTION-NEXT:   [[TRUNC:%[0-9]+]]:gpr(s8) = G_TRUNC [[COPY1]](s32)
-  ; PRESELECTION-NEXT:   [[ASSERT_ZEXT:%[0-9]+]]:gpr(s8) = G_ASSERT_ZEXT [[TRUNC]], 1
   ; PRESELECTION-NEXT:   [[C:%[0-9]+]]:gpr(s32) = G_CONSTANT i32 0
   ; PRESELECTION-NEXT:   [[C1:%[0-9]+]]:gpr(s32) = G_CONSTANT i32 100000
   ; PRESELECTION-NEXT:   [[CONSTANT_FOLD_BARRIER:%[0-9]+]]:gpr(s32) = G_CONSTANT_FOLD_BARRIER [[C1]]
-  ; PRESELECTION-NEXT:   [[ANYEXT:%[0-9]+]]:gpr(s32) = G_ANYEXT [[ASSERT_ZEXT]](s8)
   ; PRESELECTION-NEXT:   [[C2:%[0-9]+]]:gpr(s32) = G_CONSTANT i32 1
-  ; PRESELECTION-NEXT:   [[AND:%[0-9]+]]:gpr(s32) = G_AND [[ANYEXT]], [[C2]]
+  ; PRESELECTION-NEXT:   [[AND:%[0-9]+]]:gpr(s32) = G_AND [[COPY1]], [[C2]]
   ; PRESELECTION-NEXT:   G_BRCOND [[AND]](s32), %bb.3
   ; PRESELECTION-NEXT:   G_BR %bb.2
   ; PRESELECTION-NEXT: {{  $}}
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir b/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir
index 62865bcfdf081..ce02aa380efe1 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/legalize-simple.mir
@@ -136,6 +136,47 @@ body:             |
     $x0 = COPY %3(s64)
     RET_ReallyLR implicit $x0
 
+...
+---
+name:            testExtOfHintOfTrunc
+body:             |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: testExtOfHintOfTrunc
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: $x0 = COPY [[COPY]](s64)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s8) = G_TRUNC %0(s64)
+    %2:_(s8) = G_ASSERT_ZEXT %1(s8), 1
+    %3:_(s64) = G_ANYEXT %2(s8)
+    $x0 = COPY %3(s64)
+    RET_ReallyLR implicit $x0
+
+...
+---
+name:            testExtOfCopyAndHintOfTrunc
+body:             |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: testExtOfCopyAndHintOfTrunc
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: $x0 = COPY [[COPY]](s64)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s8) = G_TRUNC %0(s64)
+    %2:_(s8) = COPY %1(s8)
+    %3:_(s8) = G_ASSERT_ZEXT %2(s8), 1
+    %4:_(s64) = G_ANYEXT %3(s8)
+    $x0 = COPY %4(s64)
+    RET_ReallyLR implicit $x0
+
 ...
 ---
 name:            testExtOf2CopyOfTrunc
@@ -158,3 +199,26 @@ body:             |
     RET_ReallyLR implicit $x0
 
 ...
+---
+name:            testUnmergeHintOfTrunc
+body:             |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: testUnmergeHintOfTrunc
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+    ; CHECK-NEXT: [[UV:%[0-9]+]]:_(s8), [[UV1:%[0-9]+]]:_(s8), [[UV2:%[0-9]+]]:_(s8), [[UV3:%[0-9]+]]:_(s8), [[UV4:%[0-9]+]]:_(s8), [[UV5:%[0-9]+]]:_(s8), [[UV6:%[0-9]+]]:_(s8), [[UV7:%[0-9]+]]:_(s8) = G_UNMERGE_VALUES [[COPY]](s64)
+    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(s64) = G_ANYEXT [[UV1]](s8)
+    ; CHECK-NEXT: $x0 = COPY [[ANYEXT]](s64)
+    ; CHECK-NEXT: RET_ReallyLR implicit $x0
+    %0:_(s64) = COPY $x0
+    %1:_(s16) = G_TRUNC %0(s64)
+    %2:_(s16) = G_ASSERT_ZEXT %1(s16), 1
+    %3:_(s8), %4:_(s8) = G_UNMERGE_VALUES %2(s16)
+    %5:_(s64) = G_ANYEXT %4(s8)
+    $x0 = COPY %5(s64)
+    RET_ReallyLR implicit $x0
+
+...

Copy link
Contributor

@aemerson aemerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@arsenm arsenm merged commit f686479 into llvm:main Jan 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants