target ABI: call parameters handling

Hi,

We have many times now had severe problems relating to parameter passing where the signext/zeroext attribute has been forgotten when building the LLVM I/R call instruction somewhere during compilation. The last example of this is MLIR: some function related to async runtime takes an int32_t as argument, but the place that builds the call is not adding the signext attribute on that parameter. As a result wrong code(!) resulted on SystemZ and just out of luck we happened to discover this when building MLIR with gcc (bug hides with clang). Details at 51898 – [MLIR] WRONG code: mlir-opt does not add SExt/ZExt attributes on call parameters.

The first question one might ask about this is why is there no assert catching this in the SystemZ (or any other target with the same ABI handling of arguments)? The reason is that there is a third type of integer parameter: struct passed in register. So if there is neither a signext or a zeroext attribute on an i32 argument, it must be assumed that it contains a struct (of four bytes, for example).

I personally feel there is something missing here and a shame that this very serious problem lives on. I totally understand that people working on a target that does not have this ABI requirement may not be aware that their new code causes wrong-code on some platforms. Therefore I think it all the more important that they see some test failing as soon as they make the mistake.

In order to implement the assert in the backend one idea might be to give the struct-in-reg parameter an explicit attribute. Would it be reasonable/doable to invent a new attribute such as StructArg and add it on all such arguments throughout the whole LLVM code base? Then we could assert for signext/zeroext/structarg and eliminate this type of errors.

Some examples:

The SystemZ ABI requires that all integer parameters always are to be extended to full register width (64-bit), either signed or unsigned depending on the type.

-- For exampe, this function is loading 32-bits which are passed to another function:

int bar(int *a) {
B return foo(*a);
}

=> LLVM I/R:

B %0 = load i32, i32* %a
B %call = tail call signext i32 @foo(i32 signext %0)

=> SystemZ MachineInstructions:

B %1:gr64bit = LGF %0:addr64bit, 0, $noreg :: (load (s32) from %ir.a, !tbaa !2)
B $r2d = COPY %1:gr64bit
B CallJG @foo, implicit $r2d

The important mechanism here is that on LLVM I/R the 'signext' attribute is added to the parameter %0. That way the backend knows that a sign extension is needed (there is no other way to know the signedness), and emits the sign-extending load (LGF).

-- A matching example, on the callee side would be:

long bar(int a) {
B return ((long) a);
}

=>

B %conv = sext i32 %a to i64
B ret i64 %conv

=>

B Return implicit $r2d

The 32-bit ingoing parameter is per the ABI required to already have been sign-extended so the backend can simply do nothing. NB! This leads to wrong code if the caller forgets about that :slight_smile:

-- A struct in register:

struct S {
B short el0;
B short el1;
};

int foo(struct S arg);

void bar(short c) {
B struct S a = {c, c + 1};
B foo (a);
}

=> LLVM I/R

define void @bar(i16 signext %c) local_unnamed_addr #0 {

B %add = add i16 %c, 1
B %a.sroa.4.0.insert.ext = zext i16 %add to i32
B %a.sroa.0.0.insert.ext = zext i16 %c to i32
B %a.sroa.0.0.insert.shift = shl nuw i32 %a.sroa.0.0.insert.ext, 16
B %a.sroa.0.0.insert.insert = or i32 %a.sroa.0.0.insert.shift, %a.sroa.4.0.insert.ext
B %call = tail call signext i32 @foo(i32 %a.sroa.0.0.insert.insert) #2
B ret void
}

The i32 passed to foo does not have any attribute set, and the backend does not extend it. An 'int' gets the same treatment without the signext attribute set, which is then wrong... :frowning:

/Jonas

Thanks for raising the issue, it has been an ongoing problem.

I think there are a couple things here:

  • LLVM IR integers don’t carry signedness information. In general, LLVM IR doesn’t carry full C prototype information (_Complex, unions, etc), it has to be grafted on with attributes, and this burden is imposed on every IR producer.
  • LLVM values performance over safety: the backend will pass arbitrary values in high bits of registers for any target (including x86_64, see this example where RDI is not truncated: https://gcc.godbolt.org/z/bxToE5nPv)

I don’t think it would be reasonable for the SystemZ backend to abort every time it is asked to code generate an i32 argument that lacks an attribute. There are simply too many IR producers (frontends and instrumentation tools) that will pass unannotated i32 values to calls. It’s not feasible to fix them all, and aborting just means these producers will have to be updated whenever they are ported to SystemZ or any other target adopting the same practice.

I think it would instead be reasonable to pick a safer default for the target, like always zero extending unannotated parameters on both the caller side and the callee side. This is less efficient than only extending once in the caller, but it makes it much easier to retarget an IR producer over to SystemZ. It makes the attributes into performance improvements, not correctness requirements. I suspect this will not actually break passing a 4 byte struct, it just makes it less efficient.

As for the suggestion to add an explicit structarg attribute, can this be generalized to an attribute that marks the high bits as undef? So, IR producers will have a portable way to explicitly request the current behavior. This is kind of like anyext, but not really, more like undefext, uext, hiundef or calleemustextend.

What do you think?

> I don't think it would be reasonable for the SystemZ backend to
> abort every time it is asked to code generate an i32 argument that
> lacks an attribute. There are simply too many IR producers
> (frontends and instrumentation tools) that will pass unannotated i32
> values to calls. It's not feasible to fix them all, and aborting
> just means these producers will have to be updated whenever they are
> ported to SystemZ or any other target adopting the same practice.
>
> I think it would instead be reasonable to pick a safer default for
> the target, like always zero extending unannotated parameters on
> both the caller side and the callee side. This is less efficient
> than only extending once in the caller, but it makes it much easier
> to retarget an IR producer over to SystemZ. It makes the attributes
> into performance improvements, not correctness requirements. I
> suspect this will not actually break passing a 4 byte struct, it
> just makes it less efficient.

Unfortunately there is no "safe" default. If the callee expects
a signed value, the argument has to be sign-extended; if the
callee expects an unsigned value, the argument has to be zero-
extended. Using the wrong extension is just as much a codegen
bug as not extending at all.

E.g. if the callee expects an "int" argument in a register,
which is then used as index in a (64-bit) address, generated
code may use that register without explicit extension, assuming
the caller already sign-extended the value. If the caller
passes -1, and wrongly zero-extends instead of sign-extending,
the resulting address will be wrong.

> As for the suggestion to add an explicit structarg attribute, can
> this be generalized to an attribute that marks the high bits as
> undef? So, IR producers will have a portable way to explicitly
> request the current behavior. This is kind of like anyext, but not
> really, more like undefext, uext, hiundeforcalleemustextend.

Agreed, this would be more general.

Bye,
Ulrich

>
> > I think it would instead be reasonable to pick a safer default for
> > the target, like always zero extending unannotated parameters on
> > both the caller side and the callee side. This is less efficient
> > than only extending once in the caller, but it makes it much easier
> > to retarget an IR producer over to SystemZ. It makes the attributes
> > into performance improvements, not correctness requirements. I
> > suspect this will not actually break passing a 4 byte struct, it
> > just makes it less efficient.
>
> Unfortunately there is no "safe" default. If the callee expects
> a signed value, the argument has to be sign-extended; if the
> callee expects an unsigned value, the argument has to be zero-
> extended. Using the wrong extension is just as much a codegen
> bug as not extending at all.

That said, of course you're right that using some extension by
default is "safer" in the sense that it'll just do the right
thing in more cases than simply not extending at all.

So maybe one option might be to implement Jonas' original idea
of an abort unless the parameter is marked either zeroext or
signext or anyext, but only under control of a command line
option enabling "strict checking". This would be off by default,
in which case the back-end would default to some extension
as per your suggestion.

The strict option could be on e.g. for the buildbot jobs, so
we'd at least have a good chance of catching missing extensions
in code that's part of the LLVM repo and checked by the bot.

Bye,
Ulrich

Hi Reid,

I don’t think it would be reasonable for the SystemZ backend to abort every time it is asked to code generate an i32 argument that lacks an attribute. There are simply too many IR producers (frontends and instrumentation tools) that will pass unannotated i32 values to calls. It’s not feasible to fix them all, and aborting just means these producers will have to be updated whenever they are ported to SystemZ or any other target adopting the same practice.

I think it would instead be reasonable to pick a safer default for the target, like always zero extending unannotated parameters on both the caller side and the callee side. This is less efficient than only extending once in the caller, but it makes it much easier to retarget an IR producer over to SystemZ. It makes the attributes into performance improvements, not correctness requirements. I suspect this will not actually break passing a 4 byte struct, it just makes it less efficient.

As Uli explained, this is not an optimization but actually really necessary in order to perform the right extension. I don’t think there is any way around fixing this the slow and arduous way :-/

As for the suggestion to add an explicit structarg attribute, can this be generalized to an attribute that marks the high bits as undef? So, IR producers will have a portable way to explicitly request the current behavior. This is kind of like anyext, but not really, more like undefext, uext, hiundef or calleemustextend.

What do you think?

I agree that some more general attribute could work as well to mean for a parameter that it is not an integer value and no extension is therefore required per the ABI. The important thing about the name is that whoever builds the call instruction needs to be aware that it is only for non-integer values - the other trap to fall into I guess might be that ‘anyext’ could be added carelessly on integers when it seems that it might not matter, even when it really does per the ABI… :slight_smile: Maybe the name ‘anyext’ would make people think that it will be extended one way or the other, which is not true. I think I like your ‘hiundef’ better… Or maybe ‘nonint’, or ‘noext’?

Do you agree we need to do this, and what would the next step be towards getting this in place? If we decide on the attribute to use, I could implement the assertion and start filing bugs and hopefully get some help with them as well… Even though this may not be “feasible”, I hope this would eventually lead to having this working correctly, or is there any reason not?

/Jonas

Hi Reid,

I don’t think it would be reasonable for the SystemZ backend to abort every time it is asked to code generate an i32 argument that lacks an attribute. There are simply too many IR producers (frontends and instrumentation tools) that will pass unannotated i32 values to calls. It’s not feasible to fix them all, and aborting just means these producers will have to be updated whenever they are ported to SystemZ or any other target adopting the same practice.

I think it would instead be reasonable to pick a safer default for the target, like always zero extending unannotated parameters on both the caller side and the callee side. This is less efficient than only extending once in the caller, but it makes it much easier to retarget an IR producer over to SystemZ. It makes the attributes into performance improvements, not correctness requirements. I suspect this will not actually break passing a 4 byte struct, it just makes it less efficient.

As Uli explained, this is not an optimization but actually really necessary in order to perform the right extension. I don’t think there is any way around fixing this the slow and arduous way :-/

Well, the annotation isn’t strictly necessary. The backend can choose to emit an extension for short integer arguments in the absence of annotations. The value would be to reduce the number of SystemZ-specific changes to IR producers. It would be a shame if i32 arguments just didn’t work out of the box on SystemZ. That will be an ongoing source of IR producer bugs.

In any case, I don’t feel like my argument is all that compelling. If you and the SystemZ owners feel like this is the best plan, go for it.

The same problem exists for i16 and i8 arguments on x86, BTW: https://gcc.godbolt.org/z/7dds6sfv8 These are much less common argument types, but you might consider expanding the scope of this proposal.

As for the suggestion to add an explicit structarg attribute, can this be generalized to an attribute that marks the high bits as undef? So, IR producers will have a portable way to explicitly request the current behavior. This is kind of like anyext, but not really, more like undefext, uext, hiundef or calleemustextend.

What do you think?

I agree that some more general attribute could work as well to mean for a parameter that it is not an integer value and no extension is therefore required per the ABI. The important thing about the name is that whoever builds the call instruction needs to be aware that it is only for non-integer values - the other trap to fall into I guess might be that ‘anyext’ could be added carelessly on integers when it seems that it might not matter, even when it really does per the ABI… :slight_smile: Maybe the name ‘anyext’ would make people think that it will be extended one way or the other, which is not true. I think I like your ‘hiundef’ better… Or maybe ‘nonint’, or ‘noext’?

Do you agree we need to do this, and what would the next step be towards getting this in place? If we decide on the attribute to use, I could implement the assertion and start filing bugs and hopefully get some help with them as well… Even though this may not be “feasible”, I hope this would eventually lead to having this working correctly, or is there any reason not?

I think I like noext, it fits well with the existing attributes.

As for next steps, we need to settle on the name. If you like noext, send a patch, CC some other folks and see if they like the name.

> Well, the annotation isn't strictly necessary. The backend can
> choose to emit an extension for short integer arguments in the
> absence of annotations.

As I commented in another mail, unfortunately this is not true.
To ensure correct codegen in all cases, we need to know *which*
extension the callee expects: zero- or sign-extension. The
backend cannot know this without being told by the frontend.

Bye,
Ulrich

Maybe this is just a wording thing, but I do believe my statement is true: the backend is absolutely free to guess if it wants to, and requiring attributes doesn’t guarantee correctness. It may just move the bug to the frontend. I acknowledge that there are risks to guessing. I just think it is important to consider alternative designs and weigh the pros and cons.

>> As I commented in another mail, unfortunately this is not true.
>> To ensure correct codegen in all cases, we need to know *which*
>> extension the callee expects: zero- or sign-extension. The
>> backend cannot know this without being told by the frontend.

> Maybe this is just a wording thing, but I do believe my statement is
> true: the backend is absolutely free to guess if it wants to, and
> requiring attributes doesn't guarantee correctness. It may just move
> the bug to the frontend. I acknowledge that there are risks to
> guessing. I just think it is important to consider alternative
> designs and weigh the pros and cons.

I guess it's indeed just wording, and we agree on the substance.

Thanks,
Ulrich

Sorry for the delay on this… Please find my initial patch proposal at .

/Jonas