This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Serialize function calls in function arguments.
AcceptedPublic

Authored by chapuni on May 15 2022, 5:06 AM.

Details

Summary

Evaluation odering in function call arguments is implementation-dependent.
In fact, gcc evaluates bottom-top and clang does top-bottom.

I confirmed, with this change, that every getSCEV() is called idetically
between clang and gcc in the compilation of the LLVM tree.

Fixes #55283

FIXME: I didn't fix all of the matter.

Diff Detail

Event Timeline

chapuni created this revision.May 15 2022, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 5:07 AM
chapuni requested review of this revision.May 15 2022, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 5:07 AM
Allen added a subscriber: Allen.May 15 2022, 1:09 PM
nikic accepted this revision.May 16 2022, 12:09 PM

LGTM with some code style suggestions.

llvm/lib/Analysis/ScalarEvolution.cpp
1807

I'd just reassign LHS and RHS here (and below). The LHSz/RHSz names are bit unusual.

3127

const SCEV * here and elsewhere. This auto is not saving a lot of characters :)

7576

Please move these as separate assignments before the if (here and below).

This revision is now accepted and ready to land.May 16 2022, 12:09 PM
chapuni marked an inline comment as done.May 18 2022, 7:25 AM

At the moment, I have committed llvmorg-15-init-10782-g6ca7eb2c6d7d

chapuni added inline comments.May 18 2022, 7:38 AM
llvm/lib/Analysis/ScalarEvolution.cpp
1807

I was afraid since LHS and RHS are captured and assigned by matchURem(const SCEV*, const SCEV*&, const SCEV*&)
I thought they might be dedicated to matchURem().

How about;

  • Rename original LHS,RHS to LHSURem,RHSURem
  • Use LHS,RHS as generic instead of my LHSz,RHSz
7576

I avoided to evaluate RHS if the former condition (LHS) was false.
Could I split conditions? Or evaluate both LHS and RHS before conditions?

LHS =  = getSCEV(U->getOperand(0);
if (isKnownNonNegative(LHS))) {
  RHS = getSCEV(U->getOperand(1);
  if (isKnownNonNegative(RHS)))
    return getUDivExpr(LHS, RHS);
}
nikic added inline comments.May 30 2022, 4:21 AM
llvm/lib/Analysis/ScalarEvolution.cpp
1807

I don't think the capture here matters. These are just out parameters for matchURem, it's fine to reuse the variables.