Page MenuHomePhabricator

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

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

Details

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

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
emilio added a comment.Dec 8 2019, 4:34 PM

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

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.

emilio added a comment.Dec 8 2019, 5:00 PM

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?

emilio updated this revision to Diff 232751.Dec 8 2019, 5:35 PM

update tests

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

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.

pengfei added a subscriber: pengfei.Feb 3 2020, 6:51 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
emilio added a comment.Apr 2 2022, 4:40 AM

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.

lebedev.ri requested changes to this revision.Apr 2 2022, 4:49 AM

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.

Yep, that seems more like it.