[x86] Don't assume sign-extension of arguments smaller than 32-bits.
Changes PlannedPublic
Actions

Authored by emilio on Dec 8 2019, 4:33 PM.

Details

Reviewers

craig.topper
RKSimon
lebedev.ri

Summary

Even though llvm currently will sign-extend in the caller, this is not
guaranteed by the ABI, and thus the callee cannot assume it.

This fixes PR44228 and PR12207.

I still have a few tests to update, but I want to sanity-check this is
reasonable before doing so.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

emilio created this revision.Dec 8 2019, 4:33 PM

Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2019, 4:33 PM

Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript

Could I get some feedback on this? Does this look reasonable?

xbolva00 added reviewers: craig.topper, RKSimon.Dec 8 2019, 4:38 PM

xbolva00 added a subscriber: xbolva00.

xbolva00 added inline comments.

llvm/test/CodeGen/X86/x86-64-arg.ll
1	Why deleted? You can update it: Replace this line with: ; RUN: llc < %s -mtriple=x86_64-unknown-unknown \| FileCheck %s and run: utils/update_llc_test_checks.py llvm/test/CodeGen/X86/x86-64-arg.ll

emilio marked an inline comment as done.Dec 8 2019, 5:00 PM

emilio added inline comments.

llvm/test/CodeGen/X86/x86-64-arg.ll
1	This was testing exactly for the wrong assumption, so it seemed like it was not worth keeping. But I'm happy to update it if you prefer.

Oh, and thanks for the utils/update_llc_test_checks.py tip, I was updating them manually and it was driving me crazy :)

Shouldn’t we instead stop the sext and zext attributes from being added by the frontend?

update tests

In D71178#1774433, @craig.topper wrote:

Shouldn’t we instead stop the sext and zext attributes from being added by the frontend?

Heh, yeah, I was just going to comment on that, here's my WIP comment:

While I looked through more tests.I think this patch, even though it produces the expected behavior, is wrong... In particular, it seems clang is generating ir with the zeroext / signext annotation, and _that_ is the thing to fix, not this, which just disables the optimization the annotation enables.
(I think. My llvm-foo is not the strongest)

So yeah, I think you're right. I'll look into where does clang generate this. Thanks!

emilio planned changes to this revision.Dec 8 2019, 5:37 PM

So I started doing that, and D72742 looked different enough that I thought I would submit it separately. Do you think that's on the right track? Or have I missed something? Thanks.

emilio mentioned this in D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI..Feb 3 2020, 5:29 AM

pengfei added a subscriber: pengfei.Feb 3 2020, 6:51 PM

LiuChen3 mentioned this in D122963: [X86] Extend the integer parameter if the function isn't local linked.Apr 1 2022, 10:47 PM

This seems backwards.
Who added those signext/zeroext attrs to the function arguments?
Presumably they just shouldn't be added if that is not what ABI states.

Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2022, 2:31 AM

Herald added a subscriber: StephenFan. · View Herald Transcript

In D71178#3424386, @lebedev.ri wrote:

This seems backwards.
Who added those signext/zeroext attrs to the function arguments?
Presumably they just shouldn't be added if that is not what ABI states.

Yes, see D72742 for the right approach.

In D71178#3424445, @emilio wrote:

In D71178#3424386, @lebedev.ri wrote:

This seems backwards.
Who added those signext/zeroext attrs to the function arguments?
Presumably they just shouldn't be added if that is not what ABI states.

Yes, see D72742 for the right approach.