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.Mon, Feb 3, 6:51 PM