-
Notifications
You must be signed in to change notification settings - Fork 15k
[PredicateInfo] Reformat PT_Switch's annotation as valid comments #165249
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
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: Kunqiu Chen (Camsyn) ChangesPreviously, PredicateInfo brutally annotated f'"; switch predicate info ... CaseValue: ... Switch: {*PS->Switch} Edge: ... RenamedOp: ..."'However, the ; switch predicate info { CaseValue: i32 1 Switch: switch i32 %x, label %default [
i32 0, label %case0
i32 1, label %case1
i32 2, label %case0
i32 3, label %case3
i32 4, label %default
] Edge: [label %sw,label %case1], RenamedOp: %x }
x.0 = bitcast i32 %x to i32This patch reformats the f'"; switch predicate info ... CaseValue: ... Edge: ... Switch: \n{with_lines_commented(*PS->Switch)}\n; RenamedOp: ..."', e.g., ; switch predicate info { CaseValue: i32 1 Edge: [label %sw,label %case1] Switch:
; switch i32 %x, label %default [
; i32 0, label %case0
; i32 1, label %case1
; i32 2, label %case0
; i32 3, label %case3
; i32 4, label %default
; ]
; , RenamedOp: %x }
x.0 = bitcast i32 %x to i32
Full diff: https://github.com/llvm/llvm-project/pull/165249.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/PredicateInfo.cpp b/llvm/lib/Transforms/Utils/PredicateInfo.cpp
index 978d5a25a57c8..4f0f756ecc3ca 100644
--- a/llvm/lib/Transforms/Utils/PredicateInfo.cpp
+++ b/llvm/lib/Transforms/Utils/PredicateInfo.cpp
@@ -14,6 +14,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Analysis/AssumptionCache.h"
#include "llvm/IR/AssemblyAnnotationWriter.h"
#include "llvm/IR/Dominators.h"
@@ -25,6 +26,7 @@
#include "llvm/Support/Debug.h"
#include "llvm/Support/DebugCounter.h"
#include "llvm/Support/FormattedStream.h"
+#include "llvm/Support/raw_ostream.h"
#define DEBUG_TYPE "predicateinfo"
using namespace llvm;
using namespace PatternMatch;
@@ -812,11 +814,23 @@ class PredicateInfoAnnotatedWriter : public AssemblyAnnotationWriter {
OS << "]";
} else if (const auto *PS = dyn_cast<PredicateSwitch>(PI)) {
OS << "; switch predicate info { CaseValue: " << *PS->CaseValue
- << " Switch:" << *PS->Switch << " Edge: [";
+ << " Edge: [";
PS->From->printAsOperand(OS);
OS << ",";
PS->To->printAsOperand(OS);
- OS << "]";
+ OS << "] Switch:\n";
+ // A switch might cross > 1 lines, we should add the comment prefix ';'
+ // for each line
+ std::string SwitchStr;
+ {
+ llvm::raw_string_ostream SwitchOS(SwitchStr);
+ PS->Switch->print(SwitchOS);
+ }
+ SmallVector<StringRef, 8> Lines;
+ StringRef(SwitchStr).split(Lines, '\n');
+ for (const auto &Line : Lines)
+ OS << "; " << Line << "\n";
+ OS << "; ";
} else if (const auto *PA = dyn_cast<PredicateAssume>(PI)) {
OS << "; assume predicate info {"
<< " Comparison:" << *PA->Condition;
diff --git a/llvm/test/Transforms/Util/PredicateInfo/condprop.ll b/llvm/test/Transforms/Util/PredicateInfo/condprop.ll
index 0235732b95a83..256d0d908ec1e 100644
--- a/llvm/test/Transforms/Util/PredicateInfo/condprop.ll
+++ b/llvm/test/Transforms/Util/PredicateInfo/condprop.ll
@@ -133,19 +133,13 @@ define void @test4(i1 %b, i32 %x) {
; CHECK-LABEL: @test4(
; CHECK-NEXT: br i1 [[B:%.*]], label [[SW:%.*]], label [[CASE3:%.*]]
; CHECK: sw:
-; CHECK: i32 0, label [[CASE0:%.*]]
-; CHECK-NEXT: i32 1, label [[CASE1:%.*]]
-; CHECK-NEXT: i32 2, label [[CASE0]]
-; CHECK-NEXT: i32 3, label [[CASE3]]
-; CHECK-NEXT: i32 4, label [[DEFAULT:%.*]]
-; CHECK-NEXT: ] Edge: [label [[SW]],label %case1], RenamedOp: [[X:%.*]] }
-; CHECK-NEXT: [[X_0:%.*]] = bitcast i32 [[X]] to i32
-; CHECK-NEXT: switch i32 [[X]], label [[DEFAULT]] [
-; CHECK-NEXT: i32 0, label [[CASE0]]
-; CHECK-NEXT: i32 1, label [[CASE1]]
-; CHECK-NEXT: i32 2, label [[CASE0]]
-; CHECK-NEXT: i32 3, label [[CASE3]]
-; CHECK-NEXT: i32 4, label [[DEFAULT]]
+; CHECK: [[X_0:%.*]] = bitcast i32 [[X:%.*]] to i32
+; CHECK-NEXT: switch i32 [[X]], label [[DEFAULT:%.*]] [
+; CHECK-NEXT: i32 0, label [[CASE0:%.*]]
+; CHECK-NEXT: i32 1, label [[CASE1:%.*]]
+; CHECK-NEXT: i32 2, label [[CASE0]]
+; CHECK-NEXT: i32 3, label [[CASE3]]
+; CHECK-NEXT: i32 4, label [[DEFAULT]]
; CHECK-NEXT: ]
; CHECK: default:
; CHECK-NEXT: call void @bar(i32 [[X]])
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure it really makes sense to print the full switch here in the first place. Can we just drop this? The printed edge already identifies this uniquely.
I print the complete I think it's okay to eliminate such printing, as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| ; CHECK-NEXT: i32 2, label [[CASE0]] | ||
| ; CHECK-NEXT: i32 3, label [[CASE3]] | ||
| ; CHECK-NEXT: i32 4, label [[DEFAULT:%.*]] | ||
| ; CHECK-NEXT: ] Edge: [label [[SW]],label %case1], RenamedOp: [[X:%.*]] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't use UTC to generate check lines... All outputs from -passes=print-predicateinfo are dropped :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is basically just checking bitcast placement.
It would be nice to add support to UTC for testing annotated IR -- this would also be useful for MemorySSA tests. Probably just a matter of not stripping line comments in the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outputs from -passes=print-predicateinfo consist of TWO parts:
- Annotation of predicate info as comments
- no-op bitcasts
The UTC would drop all comments, i.e., the annotation part :(
However, it's too annoying to hand-write IR checks for such a big testcase.
Maybe we can add a new feature for UTC to let it check the generated comments? @dtcxzyw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is basically just checking bitcast placement.
It would be nice to add support to UTC for testing annotated IR -- this would also be useful for MemorySSA tests. Probably just a matter of not stripping line comments in the function?
I will try to support this new feature.
…vm#165249) Previously, PredicateInfo brutally annotated `PT_Switch` as follows: ```python f'"; switch predicate info ... CaseValue: ... Switch: {*PS->Switch} Edge: ... RenamedOp: ..."' ``` However, the `switch` instruction in LLVM might cross >1 lines, leading to the annotation of `PT_Switch` being **illegal comments**, e.g., ```LLVM ; switch predicate info { CaseValue: i32 1 Switch: switch i32 %x, label %default [ i32 0, label %case0 i32 1, label %case1 i32 2, label %case0 i32 3, label %case3 i32 4, label %default ] Edge: [label %sw,label %case1], RenamedOp: %x } x.0 = bitcast i32 %x to i32 ``` This patch removes the `switch` printing, reformating the `PT_Switch`'s annotation as follows: ```python f'"; switch predicate info ... CaseValue: ... Edge: ... RenamedOp: ..."' ``` , e.g., ```LLVM ; switch predicate info { CaseValue: i32 1 Edge: [label %sw,label %case1], RenamedOp: %x } x.0 = bitcast i32 %x to i32 ``` It should be Okay because `CaseValue: ...` + `Edge: ...` are meaningful enough for diagnostics, covering the necessary info provided by full switch printing (E.g., `PT_Branch` also did not print the relevant branch instruction).
…vm#165249) Previously, PredicateInfo brutally annotated `PT_Switch` as follows: ```python f'"; switch predicate info ... CaseValue: ... Switch: {*PS->Switch} Edge: ... RenamedOp: ..."' ``` However, the `switch` instruction in LLVM might cross >1 lines, leading to the annotation of `PT_Switch` being **illegal comments**, e.g., ```LLVM ; switch predicate info { CaseValue: i32 1 Switch: switch i32 %x, label %default [ i32 0, label %case0 i32 1, label %case1 i32 2, label %case0 i32 3, label %case3 i32 4, label %default ] Edge: [label %sw,label %case1], RenamedOp: %x } x.0 = bitcast i32 %x to i32 ``` This patch removes the `switch` printing, reformating the `PT_Switch`'s annotation as follows: ```python f'"; switch predicate info ... CaseValue: ... Edge: ... RenamedOp: ..."' ``` , e.g., ```LLVM ; switch predicate info { CaseValue: i32 1 Edge: [label %sw,label %case1], RenamedOp: %x } x.0 = bitcast i32 %x to i32 ``` It should be Okay because `CaseValue: ...` + `Edge: ...` are meaningful enough for diagnostics, covering the necessary info provided by full switch printing (E.g., `PT_Branch` also did not print the relevant branch instruction).
…vm#165249) Previously, PredicateInfo brutally annotated `PT_Switch` as follows: ```python f'"; switch predicate info ... CaseValue: ... Switch: {*PS->Switch} Edge: ... RenamedOp: ..."' ``` However, the `switch` instruction in LLVM might cross >1 lines, leading to the annotation of `PT_Switch` being **illegal comments**, e.g., ```LLVM ; switch predicate info { CaseValue: i32 1 Switch: switch i32 %x, label %default [ i32 0, label %case0 i32 1, label %case1 i32 2, label %case0 i32 3, label %case3 i32 4, label %default ] Edge: [label %sw,label %case1], RenamedOp: %x } x.0 = bitcast i32 %x to i32 ``` This patch removes the `switch` printing, reformating the `PT_Switch`'s annotation as follows: ```python f'"; switch predicate info ... CaseValue: ... Edge: ... RenamedOp: ..."' ``` , e.g., ```LLVM ; switch predicate info { CaseValue: i32 1 Edge: [label %sw,label %case1], RenamedOp: %x } x.0 = bitcast i32 %x to i32 ``` It should be Okay because `CaseValue: ...` + `Edge: ...` are meaningful enough for diagnostics, covering the necessary info provided by full switch printing (E.g., `PT_Branch` also did not print the relevant branch instruction).
Some analysis/transformation, e.g., predicate info/ mem ssa, insert instruction annotations as comments, referring to #165249 (comment). This PR makes UTC support checking these instruction annotations with an extra UTC option `-check-inst-comments`. E.g., Before: ```LLVM ; CHECK: [[Z_0:%.*]] = bitcast i1 [[Z]] to i1 ``` After: ```LLVM ; CHECK-NEXT: ; branch predicate info { TrueEdge: 0 Comparison: [[Z]] = and i1 [[XZ]], [[YZ]] Edge: [label [[TMP0:%.*]],label %nope], RenamedOp: [[Z]] } ; CHECK-NEXT: [[Z_0:%.*]] = bitcast i1 [[Z]] to i1 ``` This PR also regenerates all UTC-generated tests for PredicateInfo; No MemSSA test is updated, as there are no UTC-generated tests designated for `print<memoryssa>`.
Some analysis/transformation, e.g., predicate info/ mem ssa, insert instruction annotations as comments, referring to llvm/llvm-project#165249 (comment). This PR makes UTC support checking these instruction annotations with an extra UTC option `-check-inst-comments`. E.g., Before: ```LLVM ; CHECK: [[Z_0:%.*]] = bitcast i1 [[Z]] to i1 ``` After: ```LLVM ; CHECK-NEXT: ; branch predicate info { TrueEdge: 0 Comparison: [[Z]] = and i1 [[XZ]], [[YZ]] Edge: [label [[TMP0:%.*]],label %nope], RenamedOp: [[Z]] } ; CHECK-NEXT: [[Z_0:%.*]] = bitcast i1 [[Z]] to i1 ``` This PR also regenerates all UTC-generated tests for PredicateInfo; No MemSSA test is updated, as there are no UTC-generated tests designated for `print<memoryssa>`.
Previously, PredicateInfo brutally annotated
PT_Switchas follows:f'"; switch predicate info ... CaseValue: ... Switch: {*PS->Switch} Edge: ... RenamedOp: ..."'However, the
switchinstruction in LLVM might cross >1 lines, leading to the annotation ofPT_Switchbeing illegal comments, e.g.,This patch removes the
switchprinting, reformating thePT_Switch's annotation as follows:f'"; switch predicate info ... CaseValue: ... Edge: ... RenamedOp: ..."', e.g.,
It should be Okay because
CaseValue: ...+Edge: ...are meaningful enough for diagnostics, covering the necessary info provided by full switch printing (E.g.,PT_Branchalso did not print the relevant branch instruction).