Skip to content

[CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls #89707

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 4 commits into from
Apr 29, 2024
Merged

[CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls #89707

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c061c8f
[CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls
kees Apr 23, 2024
b7abf7c
add tests for -mregparm vs runtime calls
kees Apr 24, 2024
b676440
Add comment to BuildLibCalls.h with a FIXME hint about where things a…
kees Apr 24, 2024
3107e76
Update comments for greater clarity
kees Apr 27, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions clang/lib/CodeGen/CodeGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
#include "llvm/Support/xxhash.h"
#include "llvm/TargetParser/Triple.h"
#include "llvm/TargetParser/X86TargetParser.h"
#include "llvm/Transforms/Utils/BuildLibCalls.h"
#include <optional>

using namespace clang;
Expand Down Expand Up @@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext &C,
}
ModuleNameHash = llvm::getUniqueInternalLinkagePostfix(Path);
}

// Record mregparm value now so it is visible through all of codegen.
if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
getModule().addModuleFlag(llvm::Module::Error, "NumRegisterParameters",
CodeGenOpts.NumRegisterParameters);
}

CodeGenModule::~CodeGenModule() {}
Expand Down Expand Up @@ -980,11 +986,6 @@ void CodeGenModule::Release() {
NMD->addOperand(MD);
}

// Record mregparm value now so it is visible through rest of codegen.
if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
getModule().addModuleFlag(llvm::Module::Error, "NumRegisterParameters",
CodeGenOpts.NumRegisterParameters);

if (CodeGenOpts.DwarfVersion) {
getModule().addModuleFlag(llvm::Module::Max, "Dwarf Version",
CodeGenOpts.DwarfVersion);
Expand Down Expand Up @@ -4781,6 +4782,7 @@ CodeGenModule::CreateRuntimeFunction(llvm::FunctionType *FTy, StringRef Name,
}
}
setDSOLocal(F);
markRegisterParameterAttributes(F);
Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 23, 2024

Choose a reason for hiding this comment

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

This really shouldn't work this way: we should go through CodeGenModule::SetLLVMFunctionAttributes to get all the correct attributes for a function. Unfortunately, that's a bit complicated in this context because you need to construct a CGFunctionInfo, and none of the callers currently do that. Which might be more work than you really want... CC @JonPsson1 @jdoerfert @jhuber6
Copy link
Contributor Author

@kees kees Apr 24, 2024

Choose a reason for hiding this comment

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

I was trying to basically duplicate what was already done for the libcall functions. What is gained by using CGFunctionInfo? (Things already work as-is -- though generally it does looks like -mregparm support is kinda ad-hoc.)
Copy link
Contributor Author

@kees kees Apr 24, 2024

Choose a reason for hiding this comment

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

Oh, I think I see what you mean -- this is the common place where all functions should get their attributes set. Can the libcall code use this? (As in, can we remove markRegisterParameterAttributes() entirely and move the logic into SetLLVMFunctionAttributes() or is that just fantastic overkill?
Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 24, 2024

Choose a reason for hiding this comment

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

Zero/sign-extend attributes are also missing, I think. Which probably doesn't affect x86, but could have obscure effects on some targets.

Using SetLLVMFunctionAttributes here isn't really a problem, except that it takes a clang::Type, not an llvm::Type type, and we only have a conversion in the other direction. So you'd need to modify the callers. And there are a lot of callers.
Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 24, 2024

Choose a reason for hiding this comment

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

To clarify, the code in llvm/ still needs to exist because we need some approximation of the calling convention even if we don't have access to the clang AST. But clang shouldn't use it; clang's native representation of calling conventions is better.

(It's a long-standing request to try to give LLVM a representation of calling conventions that's closer to clang's representation, but it's a large problem, and nobody has really made any progress on it.)
Copy link
Contributor Author

@kees kees Apr 24, 2024

Choose a reason for hiding this comment

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

This seems like a large proposed change; is it worth it for this corner case? I think I would prefer to fix this as I have it now.
Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 24, 2024

Choose a reason for hiding this comment

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

Sign/zero-extend attributes led to real issues on some targets in the past, so it's not unlikely someone will need to address it at some point. But maybe it doesn't need to be here. I guess leave a FIXME noting it as a known issue.

I'm not really happy with exposing this interface on BuildLibCalls.h, since it's really a pretty big footgun (due to the zero/sign-extension issue I mentioned). But copy-pasting the code is worse, so... I guess leave a comment in the header.
Copy link
Contributor Author

@kees kees Apr 24, 2024

Choose a reason for hiding this comment

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

Comment added. Is this what you had in mind?
Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 26, 2024

Choose a reason for hiding this comment

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

Suggested text on the clang change:

FIXME: We should use CodeGenModule::SetLLVMFunctionAttributes() instead of trying to approximate the attributes using the LLVM function signature. This requires revising the API of CreateRuntimeFunction().
Copy link
Contributor Author

@kees kees Apr 27, 2024

Choose a reason for hiding this comment

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

Ah-ha, thanks! Okay, I've updated the comments with just a minor tweak.
}
}

Expand Down
18 changes: 18 additions & 0 deletions clang/test/CodeGen/regparm-flag.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
// RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 4 %s -emit-llvm -o - | FileCheck %s
// RUN: %clang_cc1 -triple i386-unknown-unknown -fsanitize=array-bounds %s -emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME0
// RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 1 -fsanitize=array-bounds %s -emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME1
// RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 2 -fsanitize=array-bounds %s -emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME2
// RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 3 -fsanitize=array-bounds %s -emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME2

void f1(int a, int b, int c, int d,
int e, int f, int g, int h);
Expand All @@ -13,7 +17,21 @@ void f0(void) {
f2(1, 2);
}

struct has_array {
int a;
int b[4];
int c;
};

int access(struct has_array *p, int index)
{
return p->b[index];
}

// CHECK: declare void @f1(i32 inreg noundef, i32 inreg noundef, i32 inreg noundef, i32 inreg noundef,
// CHECK: i32 noundef, i32 noundef, i32 noundef, i32 noundef)
// CHECK: declare void @f2(i32 noundef, i32 noundef)

// RUNTIME0: declare void @__ubsan_handle_out_of_bounds_abort(ptr, i32)
// RUNTIME1: declare void @__ubsan_handle_out_of_bounds_abort(ptr inreg, i32)
// RUNTIME2: declare void @__ubsan_handle_out_of_bounds_abort(ptr inreg, i32 inreg)
7 changes: 7 additions & 0 deletions llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ namespace llvm {
LibFunc TheLibFunc, AttributeList AttributeList,
FunctionType *Invalid, ArgsTy... Args) = delete;

// Handle -mregparm for the given function.
// FIXME: This should likely be implemented in
// CodeGenModule::SetLLVMFunctionAttributes() since callers of
// markRegisterParameterAttributes() will not have gotten appropriate
// attributes for things like sign/zero-extension, etc.
Copy link
Collaborator

@efriedma-quic efriedma-quic Apr 26, 2024

Choose a reason for hiding this comment

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

This doesn't really properly capture the divide between clang, the frontend, and LLVM the optimizer.

What we want here is basically a warning: this interface shouldn't be used unless you know exactly what you're doing. Then a FIXME on the clang side to indicate we plan to revise the CreateRuntimeFunction() interface at some point.

Suggested text on the LLVM API:

Note that this function is a rough approximation that only works for simple function signatures. Note this does not apply other relevant attributes for function signatures, including sign/zero-extension for arguments and return values.
void markRegisterParameterAttributes(Function *F);

/// Check whether the library function is available on target and also that
/// it in the current Module is a Function with the right type.
bool isLibFuncEmittable(const Module *M, const TargetLibraryInfo *TLI,
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Utils/BuildLibCalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,7 +1255,7 @@ static void setRetExtAttr(Function &F,
}

// Modeled after X86TargetLowering::markLibCallAttributes.
static void markRegisterParameterAttributes(Function *F) {
void llvm::markRegisterParameterAttributes(Function *F) {
if (!F->arg_size() || F->isVarArg())
return;

Expand Down