Page MenuHomePhabricator

LSR: Fix PR33514
ClosedPublic

Authored by evstupac on Aug 1 2017, 12:54 PM.

Details

Summary

The patch restrict multiplication of ICmpZero formula by any constant if there is a pointer type register (p) in the formula.
If not restricted it can potentially cause expand of SCEV : C * p, which is undefined (if C != 1 or 0).

Diff Detail

Repository
rL LLVM

Event Timeline

evstupac created this revision.Aug 1 2017, 12:54 PM
qcolombet edited edge metadata.Aug 1 2017, 1:29 PM

Hi Evgeny,

Multiplying pointers is indeed illegal in the IR, but instead of just dropping them, would it make sense to keep them with the proper ptrtoint casts?

Also, if that is not something we are supposed to do, I think it would make sense to have SCEV complain when we are trying to do that.

Cheers,
-Quentin

Hi Quentin,

Multiplying pointers is indeed illegal in the IR, but instead of just dropping them, would it make sense to keep them with the proper ptrtoint casts?

We could do this, however I have some concerns:

  1. If we convert pointer to int we can miss some optimizations. For example comparison of pointer == 1 is always false, comparison of int == 1 not.
  2. If we insert more converts we should raise Formula cost somehow (most likely leading to the formula drop).

Also, if that is not something we are supposed to do, I think it would make sense to have SCEV complain when we are trying to do that.

SCEV expands to undefined value and it seems good enough. The expansion could be in dead code for example - so assert or error is too strict.

It is hard to imagine a case when C*p will be in the best solution (so that C*p is reused somewhere else). However that can happen if other formulas were deleted because of "too complex solution".

I've tested performance for x86.
spec2000/spec2006 are build same
Other tests that have difference in binaries got the same performance.

Thanks,
Evgeny

If we insert more converts we should raise Formula cost somehow (most likely leading to the formula drop).

Good point. Maybe mention that in the comment and we can revisit if we see test where it is needed later on.

SCEV expands to undefined value and it seems good enough. The expansion could be in dead code for example - so assert or error is too strict.

Make sense.

Other tests that have difference in binaries got the same performance.

Interesting, given we were generating invalid code, what changed there?

Other tests that have difference in binaries got the same performance.

Interesting, given we were generating invalid code, what changed there?

Something similar to what have happen to the LIT test here.
We are not generating wrong code.

  1. We generate formulas that potentially generate buggy code. The formulas cold be unused, but context for the best solution differs (especially for complex cases), with and without this formulas.
  2. Even if buggy formula is used it could happen, that expand is correct: "p1 + -1*p2" is valid when we have a mapped expand (existing instruction) for "p1 + -1*p2". However, if we generate a SCEV without existing map to instruction, we need to create a new instruction to expand the SCEV. -1*p2 + p1 could be an issue.
qcolombet accepted this revision.Aug 3 2017, 12:58 PM

Thanks for the explanation.

LGTM

This revision is now accepted and ready to land.Aug 3 2017, 12:58 PM
This revision was automatically updated to reflect the committed changes.