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

[InstCombine] Missing signext attribute on parameter #54532

Closed
JonPsson opened this issue Mar 24, 2022 · 4 comments
Closed

[InstCombine] Missing signext attribute on parameter #54532

JonPsson opened this issue Mar 24, 2022 · 4 comments

Comments

@JonPsson
Copy link
Contributor

a() { printf("\n"); }

Instcombine replaces the call to printf with a call to putchar, but forgets to add an extension on the new parameter:

%call = call signext i32 (i8*, ...) @printf(i8* noundef getelementptr inbounds ([2 x i8], [2 x i8]* @.str, i64 0, i64 0))

=> After InstCombinePass

%putchar = call i32 @putchar(i32 10)

=>

        lhi     %r2, 10
        jg      putchar@PLT

This particular case seems perhaps harmless, but I suspect that calls to other library functions might not be: on SystemZ all outgoing parameters less than 32 bits in width must have a sign/zero extension attribute at the call site to conform to the ABI.

@efriedma-quic
Copy link
Collaborator

TargetLibraryInfo::getExtAttrForI32Param and TargetLibraryInfo::getExtAttrForI32Return return the right extension attributes for parameters and return values. We should use them from emitPutChar etc.

@efriedma-quic
Copy link
Collaborator

Also, https://reviews.llvm.org/D90760 is wrong; it should be replaced with a more appropriate fix.

@JonPsson
Copy link
Contributor Author

JonPsson commented Apr 4, 2022

TargetLibraryInfo::getExtAttrForI32Param and TargetLibraryInfo::getExtAttrForI32Return return the right extension attributes for parameters and return values. We should use them from emitPutChar etc.
Also, https://reviews.llvm.org/D90760 is wrong; it should be replaced with a more appropriate fix.

I think I see what you mean: the attribute should only be added if getExtAttrForI32Param() says it should. I proposed a patch for this at https://reviews.llvm.org/D123030, which also handles (so far) putchar. Please let me know if I am on the right track here before I continue with the many more library function declarations.

@JonPsson
Copy link
Contributor Author

JonPsson commented Apr 5, 2022

Thanks for review.

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
…tes().

getExtAttrForI32Param() is the method to be used for determining the type of
extension attribute (if any) that is to be added for a signed/unsigned
argument.

Previously, the SExt attribute was always added to the i32 ldexp* argument as
it was expected to be ignored by targets not needing it. This patch now
changes this so that it is only added for the targets that need it in the
first place.

Putchar() argument is now also extended as required by the target (SystemZ in
the test), to fix the issue below. Many more libcalls will be handled
similarly in a following patch.

Fixes llvm/llvm-project#54532.

Differential Revision: https://reviews.llvm.org/D123030

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

No branches or pull requests

5 participants