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

Make HelloWorld pass required #64

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Conversation

caojoshua
Copy link
Contributor

HelloWorld pass was not printing anything for me. This PR makes the pass required, as I see many other example passes are required. Not really sure why HelloWorld pass is skipped, but I do know this change forces the pass to be run.

archlinux [~/src/llvm-tutor]$ llvm-config --version
13.0.1
archlinux [~/src/llvm-tutor]$ uname -a
Linux archlinux 5.16.12-arch1-1 #1 SMP PREEMPT Wed, 02 Mar 2022 12:22:51 +0000 x86_64 GNU/Linux

@banach-space
Copy link
Owner

Hi @caojoshua , many thanks for sending this!

How did you generate the input .ll file? Did you use -O1? Just trying to better to understand the case in which it didn’t work for you.

-Andrzej

@caojoshua
Copy link
Contributor Author

I did not use -O1. I can see now, that with -O0, the hello-world pass does not do anything. But with -O1, I see the expected output. I can now see in the README example, -O1 is included.

I suppose this change is not necessary, so feel free to close. But maybe we want this change anyway?


Can you also help me understand why opt only runs the pass on -O1? Here is my input:

#include "stdio.h"

void foo() { printf("foobar\n"); }

int main() {
  int x = 1234;
  foo();

  return 0;
}

With -O0:

@.str = private unnamed_addr constant [8 x i8] c"foobar\0A\00", align 1

; Function Attrs: noinline nounwind optnone sspstrong uwtable
define dso_local void @foo() #0 {
  %1 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str, i64 0, i64 0))
  ret void
}

declare i32 @printf(i8*, ...) #1

; Function Attrs: noinline nounwind optnone sspstrong uwtable
define dso_local i32 @main() #0 {
  %1 = alloca i32, align 4
  %2 = alloca i32, align 4
  store i32 0, i32* %1, align 4
  store i32 1234, i32* %2, align 4
  call void @foo()
  ret i32 0
}

With -O1:

; Function Attrs: nofree nounwind sspstrong uwtable
define dso_local void @foo() local_unnamed_addr #0 {
  %1 = call i32 @puts(i8* nonnull dereferenceable(1) getelementptr inbounds ([7 x i8], [7 x i8]* @str, i64 0, i64 0))
  ret void
}

; Function Attrs: nofree nounwind sspstrong uwtable
define dso_local i32 @main() local_unnamed_addr #0 {
  %1 = call i32 @puts(i8* nonnull dereferenceable(1) getelementptr inbounds ([7 x i8], [7 x i8]* @str, i64 0, i64 0)) #2
  ret i32 0
}

@banach-space
Copy link
Owner

Thanks for the info!

Can you also help me understand why opt only runs the pass on -O1? Here is my input:

Note the presence of the optnone function attribute when using -O0. That's the key thing here and means that no pass should/will be run for the corresponding function.

But maybe we want this change anyway?

I do :) think that right HelloWorld might be a bit confusing and it should just work ™️ . Would you mind adding a bit more comments to justify the need for isRequired? Perhaps something like (feel free to use this or suggested something else):

Without isRequired returning true, this pass will be skipped for functions decorated with the optnone LLVM attribute. Note that clang -O0 decorates all functions with optnone.

-Andrzej

@caojoshua
Copy link
Contributor Author

Went with your comment. Note that I only changed HelloWorld.cpp, and not the other instances of isRequired in other passes. I don't think this comment needs to be copy pasted everywhere, as this information is clearly in the README.

@banach-space banach-space merged commit f45e989 into banach-space:main Mar 30, 2022
@banach-space
Copy link
Owner

Your contribution is very much appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants