Page MenuHomePhabricator

[InstCombine] Remove unneeded VarArg casts.
AbandonedPublic

Authored by fhahn on Dec 29 2017, 1:10 PM.

Details

Summary

This combine was suggested in D41335.

Diff Detail

Event Timeline

fhahn created this revision.Dec 29 2017, 1:10 PM
fhahn updated this revision to Diff 128345.Dec 29 2017, 1:19 PM
fhahn added reviewers: rnk, efriedma, hfinkel, RKSimon.
rnk added a reviewer: rjmccall.Jan 2 2018, 9:26 AM
rnk added a comment.Jan 2 2018, 10:39 AM

@rjmccall, does this cause problems on Darwin? I seem to recall that there are some ABIs (AArch64 Darwin) where arguments in the varargs pack are passed differently from the ones that are prototyped.

If that is a problem, do you think we can permit this simplification if we see a non-interposable definition of the callee? In this case, instcombine should "know" the true prototype of the callee and should be adjusting things so the types match.

There are many ABIs with special cases for varargs. The x86-64 Linux ABI specifically defines that vararg calls need to initialize AL, and other calls don't. The hard-float ABI on ARM Linux usually pass "double" arguments in floating-point regsiters, but varargs functions pass them in integer registers. Other ABIs might do more exotic things I'm not thinking of.

If you can prove the callee is in fact not a varargs function, you might be able to do something like this, but the type of a function declaration isn't enough to prove that.

rnk added a comment.Jan 2 2018, 12:39 PM

There are many ABIs with special cases for varargs. The x86-64 Linux ABI specifically defines that vararg calls need to initialize AL, and other calls don't. The hard-float ABI on ARM Linux usually pass "double" arguments in floating-point regsiters, but varargs functions pass them in integer registers. Other ABIs might do more exotic things I'm not thinking of.

If you can prove the callee is in fact not a varargs function, you might be able to do something like this, but the type of a function declaration isn't enough to prove that.

Makes sense. Fortunately, we can prove the callee is not varargs in exactly the circumstances that matter: when we would be able to inline, i.e. we have a non-interposable definition.

I think at least one of the MIPS ABIs does the same thing with varargs as AArch64 on Darwin.

Eli is right, the type of a function declaration is totally irrelevant under current IR semantics. The only thing that contributes to the CC of a call is the combination of (1) the type of the callee value, (2) the named calling convention, and (3) the attributes on the call site. I can declare malloc as taking three floats and returning void, and as long as I cast it to i8* (i64)* at every call site, everything should work. If you can prove that you have the dynamic definition of the callee, you can apply UB rules, but you don't have that here — you don't have a definition at all.

fhahn abandoned this revision.Jan 2 2018, 1:49 PM

Happy new year and thank you very much for having a look!

If ext_method is a definition InstCombine already gets rid of the bitcast in the test case. It sounds like this is the best we can do. I'll abandon this review. But I think it would be worth to commit the test case (with an definition of ext_method instead of declare ) to the file, as this case seems not to be covered. Unless there are any objection, I'll do that as a NFC.