[CIR] Handle NullStmt #134889
[CIR] Handle NullStmt #134889
Conversation
The handling for NullStmt was going to an error saying the statement handling wasn't implemented. It doesn't need any implementation. It is sufficient for emitSimpleStmt to just return success for that statement class. This change does that.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) ChangesThe handling for NullStmt was going to an error saying the statement handling wasn't implemented. It doesn't need any implementation. It is sufficient for emitSimpleStmt to just return success for that statement class. This change does that. Full diff: https://github.com/llvm/llvm-project/pull/134889.diff 2 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index 00d33e7feddff..072370ffeb4c8 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -57,6 +57,7 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
switch (s->getStmtClass()) {
case Stmt::BreakStmtClass:
+ case Stmt::NullStmtClass:
case Stmt::CompoundStmtClass:
case Stmt::ContinueStmtClass:
case Stmt::DeclStmtClass:
@@ -93,7 +94,6 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
case Stmt::SEHExceptStmtClass:
case Stmt::SEHFinallyStmtClass:
case Stmt::MSDependentExistsStmtClass:
- case Stmt::NullStmtClass:
case Stmt::LabelStmtClass:
case Stmt::AttributedStmtClass:
case Stmt::GotoStmtClass:
@@ -231,6 +231,11 @@ mlir::LogicalResult CIRGenFunction::emitSimpleStmt(const Stmt *s,
break;
case Stmt::ContinueStmtClass:
return emitContinueStmt(cast<ContinueStmt>(*s));
+
+ // NullStmt doesn't need any handling, but we need to say we handled it.
+ case Stmt::NullStmtClass:
+ break;
+
case Stmt::BreakStmtClass:
return emitBreakStmt(cast<BreakStmt>(*s));
case Stmt::ReturnStmtClass:
diff --git a/clang/test/CIR/CodeGen/basic.c b/clang/test/CIR/CodeGen/basic.c
index 673ff256c22af..6365dc2158138 100644
--- a/clang/test/CIR/CodeGen/basic.c
+++ b/clang/test/CIR/CodeGen/basic.c
@@ -90,3 +90,18 @@ int f3(void) {
// OGCG-NEXT: store i32 3, ptr %[[I_PTR]], align 4
// OGCG-NEXT: %[[I:.*]] = load i32, ptr %[[I_PTR]], align 4
// OGCG-NEXT: ret i32 %[[I]]
+
+// Verify null statement handling.
+void f4(void) {
+ ;
+}
+
+// CIR: cir.func @f4()
+// CIR-NEXT: cir.return
+
+// LLVM: define void @f4()
+// LLVM-NEXT: ret void
+
+// OGCG: define{{.*}} void @f4()
+// OGCG-NEXT: entry:
+// OGCG-NEXT: ret void
|
|
@llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesThe handling for NullStmt was going to an error saying the statement handling wasn't implemented. It doesn't need any implementation. It is sufficient for emitSimpleStmt to just return success for that statement class. This change does that. Full diff: https://github.com/llvm/llvm-project/pull/134889.diff 2 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index 00d33e7feddff..072370ffeb4c8 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -57,6 +57,7 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
switch (s->getStmtClass()) {
case Stmt::BreakStmtClass:
+ case Stmt::NullStmtClass:
case Stmt::CompoundStmtClass:
case Stmt::ContinueStmtClass:
case Stmt::DeclStmtClass:
@@ -93,7 +94,6 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
case Stmt::SEHExceptStmtClass:
case Stmt::SEHFinallyStmtClass:
case Stmt::MSDependentExistsStmtClass:
- case Stmt::NullStmtClass:
case Stmt::LabelStmtClass:
case Stmt::AttributedStmtClass:
case Stmt::GotoStmtClass:
@@ -231,6 +231,11 @@ mlir::LogicalResult CIRGenFunction::emitSimpleStmt(const Stmt *s,
break;
case Stmt::ContinueStmtClass:
return emitContinueStmt(cast<ContinueStmt>(*s));
+
+ // NullStmt doesn't need any handling, but we need to say we handled it.
+ case Stmt::NullStmtClass:
+ break;
+
case Stmt::BreakStmtClass:
return emitBreakStmt(cast<BreakStmt>(*s));
case Stmt::ReturnStmtClass:
diff --git a/clang/test/CIR/CodeGen/basic.c b/clang/test/CIR/CodeGen/basic.c
index 673ff256c22af..6365dc2158138 100644
--- a/clang/test/CIR/CodeGen/basic.c
+++ b/clang/test/CIR/CodeGen/basic.c
@@ -90,3 +90,18 @@ int f3(void) {
// OGCG-NEXT: store i32 3, ptr %[[I_PTR]], align 4
// OGCG-NEXT: %[[I:.*]] = load i32, ptr %[[I_PTR]], align 4
// OGCG-NEXT: ret i32 %[[I]]
+
+// Verify null statement handling.
+void f4(void) {
+ ;
+}
+
+// CIR: cir.func @f4()
+// CIR-NEXT: cir.return
+
+// LLVM: define void @f4()
+// LLVM-NEXT: ret void
+
+// OGCG: define{{.*}} void @f4()
+// OGCG-NEXT: entry:
+// OGCG-NEXT: ret void
|
| @@ -57,6 +57,7 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s, | |||
|
|
|||
| switch (s->getStmtClass()) { | |||
| case Stmt::BreakStmtClass: | |||
| case Stmt::NullStmtClass: | |||
There was a problem hiding this comment.
Classic codegen ALSO puts NoStmtClass here, might be worth doing 'while we are here'. In general, only invalid statements should get it, but is there for 'completeness' I presume? (since there is no default).
There was a problem hiding this comment.
Sure. Do you know what code leads to NoStmtClass?
There was a problem hiding this comment.
I'm about 99.9% sure that there is no code that can result in it, barring some error in the compiler. It is the 'class' of Stmt, which isn't virtual, but is effectively abstract (other than by language rule).
There was a problem hiding this comment.
I just looked at classic codegen. It puts NoStmtClass (along with CXXCatchStmtClass, SEHExceptStmtClass, SEHFinallyStmtClass, and MSDependentExistsStmtClass) in a different group just above this that leads to llvm_unreachable("invalid statement class to emit generically"); rather than llvm_unreachable("should have emitted these statements as simple"); as we have here. I can still do that here if you want, but it's currently in the group below that goes to an NYI error.
There was a problem hiding this comment.
Ah, i missed that it wasn't just the same unreachable. No real reason to do it now, but as it was a 'move this line 10 lines' sorta thing, i thought it was a decent 'while we are here'.
There was a problem hiding this comment.
I'm fine with that. Trying to avoid doing multiple things, but this is all basically doing nothing in the correct place, so it sort of makes sense.
|
|
||
| // Verify null statement handling. | ||
| void f4(void) { | ||
| ; |
There was a problem hiding this comment.
a test of for(;;); might be valuable as well? WDYT?
There was a problem hiding this comment.
There is already a test that does for (;;) {} and the semicolons in the for statement don't go through NullStmt. I was just trying to keep this focused.
There was a problem hiding this comment.
The trailing one does. The {} is a compound-stmt. I notice in class-codegen we do some stuff to figure out if the for loop is empty (or while loop/etc) that checks it IIRC.
The handling for NullStmt was going to an error saying the statement handling wasn't implemented. It doesn't need any implementation. It is sufficient for emitSimpleStmt to just return success for that statement class. This change does that.
The handling for NullStmt was going to an error saying the statement handling wasn't implemented. It doesn't need any implementation. It is sufficient for emitSimpleStmt to just return success for that statement class. This change does that.