Skip to content

[CIR] Handle NullStmt #134889

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 8, 2025
Merged

[CIR] Handle NullStmt #134889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efbec15
[CIR] Handle NullStmt
andykaylor Apr 8, 2025
c4568c7
Added test case, moved unreachable statement classes
andykaylor Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions clang/lib/CIR/CodeGen/CIRGenStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,14 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
return mlir::success();

switch (s->getStmtClass()) {
case Stmt::NoStmtClass:
case Stmt::CXXCatchStmtClass:
case Stmt::SEHExceptStmtClass:
case Stmt::SEHFinallyStmtClass:
case Stmt::MSDependentExistsStmtClass:
llvm_unreachable("invalid statement class to emit generically");
case Stmt::BreakStmtClass:
case Stmt::NullStmtClass:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Do you know what code leads to NoStmtClass?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

case Stmt::CompoundStmtClass:
case Stmt::ContinueStmtClass:
case Stmt::DeclStmtClass:
Expand Down Expand Up @@ -88,12 +95,6 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,

case Stmt::OMPScopeDirectiveClass:
case Stmt::OMPErrorDirectiveClass:
case Stmt::NoStmtClass:
case Stmt::CXXCatchStmtClass:
case Stmt::SEHExceptStmtClass:
case Stmt::SEHFinallyStmtClass:
case Stmt::MSDependentExistsStmtClass:
case Stmt::NullStmtClass:
case Stmt::LabelStmtClass:
case Stmt::AttributedStmtClass:
case Stmt::GotoStmtClass:
Expand Down Expand Up @@ -231,6 +232,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:
Expand Down
54 changes: 54 additions & 0 deletions clang/test/CIR/CodeGen/basic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,57 @@ 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) {
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

a test of for(;;); might be valuable as well? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

// 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

// Verify null statement as for-loop body.
void f5(void) {
for (;;)
;
}

// CIR: cir.func @f5()
// CIR-NEXT: cir.scope {
// CIR-NEXT: cir.for : cond {
// CIR-NEXT: %0 = cir.const #true
// CIR-NEXT: cir.condition(%0)
// CIR-NEXT: } body {
// CIR-NEXT: cir.yield
// CIR-NEXT: } step {
// CIR-NEXT: cir.yield
// CIR-NEXT: }
// CIR-NEXT: }
// CIR-NEXT: cir.return
// CIR-NEXT: }

// LLVM: define void @f5()
// LLVM: br label %[[SCOPE:.*]]
// LLVM: [[SCOPE]]:
// LLVM: br label %[[LOOP:.*]]
// LLVM: [[LOOP]]:
// LLVM: br i1 true, label %[[LOOP_STEP:.*]], label %[[LOOP_EXIT:.*]]
// LLVM: [[LOOP_STEP]]:
// LLVM: br label %[[LOOP_BODY:.*]]
// LLVM: [[LOOP_BODY]]:
// LLVM: br label %[[LOOP]]
// LLVM: [[LOOP_EXIT]]:
// LLVM: ret void

// OGCG: define{{.*}} void @f5()
// OGCG: entry:
// OGCG: br label %[[LOOP:.*]]
// OGCG: [[LOOP]]:
// OGCG: br label %[[LOOP]]