[ArgumentPromotion] Missing signext attribute on parameter.

[JonPsson]

It seems that the Argument Promotion pass forgets to add the signext attribute on a resulting i32 parameter:

// Test case that results in an outgoing int parameter which should have been                                                                                  
// sign-extended at the call-site, but which is not.  Reduced from perlio.c                                                                                    

long GlobalLongVal = 0;
struct S {
  int i;
};

static void __attribute__((noinline)) foo(struct S *s) {
  GlobalLongVal = ((long) s->i);
}

void fun() {
  struct S s = {0};
  foo(&s);
}

define internal fastcc void @foo(%struct.S* nocapture noundef readonly %s) unnamed_addr #3 {
entry:
  %i = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 0
  %0 = load i32, i32* %i, align 4, !tbaa !2
  %conv = sext i32 %0 to i64
  store i64 %conv, i64* @GlobalLongVal, align 8, !tbaa !7
  ret void
}

define dso_local void @fun() local_unnamed_addr #0 {
entry:
  %s = alloca %struct.S, align 4
  %0 = bitcast %struct.S* %s to i8*
  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0) #4
  %1 = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 0
  store i32 0, i32* %1, align 4
  call fastcc void @foo(%struct.S* noundef nonnull %s)
  call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0) #4
  ret void
}

=>

*** IR Dump After ArgumentPromotionPass on (foo) ***
; Function Attrs: mustprogress nofree noinline norecurse nosync nounwind willreturn
define internal fastcc void @foo(i32 %s.0.val) unnamed_addr #3 {
entry:
  %conv = sext i32 %s.0.val to i64
  store i64 %conv, i64* @GlobalLongVal, align 8, !tbaa !2
  ret void
}

define dso_local void @fun() local_unnamed_addr #0 {
entry:
  %s = alloca %struct.S, align 4
  %0 = bitcast %struct.S* %s to i8*
  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0) #4
  %1 = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 0
  store i32 0, i32* %1, align 4
  %2 = getelementptr %struct.S, %struct.S* %s, i64 0, i32 0
  %s.val = load i32, i32* %2, align 4, !tbaa !2
  call fastcc void @foo(i32 %s.val)
  call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0) #4
  ret void
}

The call to foo() now has an i32 argument, which I believe should have the signext attribute, given that the original struct member is 'int'.

Final output shows that the call site does not sign extend the parameter to i64, but it seems that the callee is still playing it safe and keeps the sign-extend, although per the ABI it would not strictly have to do this, I think..:

fun:                                    # @fun
# %bb.0:                                # %entry
        lhi     %r2, 0
        jg      foo@PLT
.Lfunc_end0:

foo:                                    # @foo
# %bb.0:                                # %entry
        lgfr    %r0, %r2
        stgrl   %r0, GlobalLongVal
        br      %r14

[nikic]

To clarify, is this supposed to be a miscompile or a missed optimization? The transform looks entirely correct to me -- it would be correct as long as the attributes on the call-site and the declaration match, whether they be signext, zeroext or nothing at all. We just need an agreement on how the argument is passed (and that does not need to match the platform C ABI either).

[JonPsson]

To clarify, is this supposed to be a miscompile or a missed optimization? The transform looks entirely correct to me -- it would be correct as long as the attributes on the call-site and the declaration match, whether they be signext, zeroext or nothing at all. We just need an agreement on how the argument is passed (and that does not need to match the platform C ABI either).

Please see https://lists.llvm.org/pipermail/llvm-dev/2021-October/153080.html for a discussion of the problem here, and https://reviews.llvm.org/D112942.

[efriedma-quic]

I really don't think there's a bug here separate from the proposal in D112942... if you want ArgumentPromotion to add a random extension markings to integer arguments, please just propose that as part of D112942.

[JonPsson]

Hmm.. well from a SystemZ perspective it is actually a bug to not sign extend an i32 argument (that is in this case actually known to be an 'int'). This is something that should be fixed regardless of any general verification method (as D112942). We are struggling with this as this is not typically required by target ABIs - e.g. not by X86 - so it is easily missed, but we have had wrong-code problems before because of this.

So my point here is that even without D112942 these bugs need to be fixed as they are found: it is not a new requirement. All call sites must sign or zero extend integer arguments as appropriate so that targets that have this ABI requirement can perform the extension (those who don't ignore the attribute). An exception for this is "struct in reg" which do not require any extension.

[efriedma-quic]

Integer arguments created by ArgumentPromotion are always 'struct in reg'. At least, you can consider it that way. There's no way for ArgumentPromotion to tell whether the original type in the struct was "int", "unsigned int", or something else, and it really doesn't matter.

[rnk]

Hmm.. well from a SystemZ perspective it is actually a bug to not sign extend an i32 argument

That doesn't really make sense to me. Unless I misunderstood, the generated code works. Argument promotion and other IPO passes change the ABI. It doesn't matter what conventions it uses, as long as they match between the caller and callee.

I feel like I raised my objections to the SystemZ validation scheme months ago in the llvm-dev thread, but IBM folks didn't like my proposal to pick one extension mode (probably zero) to be the default. Instead, this validation mode imposes costs on all IR producers so we can have a little more confidence that IR generators are correctly marking integer arguments as signed or unsigned, even in cases like this, where argument promotion would just have to guess the signedness.

[JonPsson]

Integer arguments created by ArgumentPromotion are always 'struct in reg'. At least, you can consider it that way. There's no way for ArgumentPromotion to tell whether the original type in the struct was "int", "unsigned int", or something else, and it really doesn't matter.

I think I understand your point now: ArgumentPromotion only transforms local functions, so it should be safe enough then to have it add the proposed NoExt attribute with D112942, telling the backend that any extension of the parameter can not be safely removed.

That doesn't really make sense to me. Unless I misunderstood, the generated code works. Argument promotion and other IPO passes change the ABI. It doesn't matter what conventions it uses, as long as they match between the caller and callee.

Yeah... (see above).

I feel like I raised my objections to the SystemZ validation scheme months ago in the llvm-dev thread, but IBM folks didn't like my proposal to pick one extension mode (probably zero) to be the default. Instead, this validation mode imposes costs on all IR producers so we can have a little more confidence that IR generators are correctly marking integer arguments as signed or unsigned, even in cases like this, where argument promotion would just have to guess the signedness.

I agree it would not be worth the trouble to do that for local functions. In these cases it is easy to just not optimize away any extension in callee.

I think the main concern and danger are when calling global functions, like a library function call. Such a function, which may not have been compiled with clang, is free to assume that any integer argument less than 64 bits have been extended as appropriate to 64 bits. Only the front end knows if that argument is signed or not, and this has to be passed to the backend to do the extension at the call site, and of course it should be the right type of extension. Having said that, we are also considering your proposal of using a default sign-extension of outgoing call-arguments in cases where this is attribute is missing. That can still lead to wrong-code though for large unsigned values, but is better than nothing.

I will update D112942 accordingly and accept this issue as closed - thanks for your input!

[rnk]

You know, I think that's probably a pretty good compromise. Thanks for proposing that solution.