Page MenuHomePhabricator

[SCEV] Pass an explicit context to computeKnownBits
Changes PlannedPublic

Authored by reames on Feb 19 2021, 12:14 PM.

Details

Reviewers
nikic
fhahn
bollu
Summary

This actually changes less than it appears. computeKnownBits will chose it's own context if one is not passed in. However, since it doesn't know which function is being queried, it can't use an context for the non-instruction case. So the only actual change in behavior here is for arguments, global values, etc..

The test changes are a bit weird, but I left them in (as opposed to rebasing over them) as I think it's relevant. The interesting instruction has to come before the assume or the ephemeral value logic ignores the assume. That's, well, a bit weird.

Diff Detail

Unit TestsFailed

TimeTest
910 msx64 windows > MLIR.Examples/Toy/Ch6::jit.toy
Script: -- : 'RUN: at line 1'; toyc-ch6 -emit=jit C:\ws\w64\llvm-project\premerge-checks\mlir\test\Examples\Toy\Ch6\jit.toy
770 msx64 windows > MLIR.Examples/Toy/Ch7::jit.toy
Script: -- : 'RUN: at line 1'; toyc-ch7 -emit=jit C:\ws\w64\llvm-project\premerge-checks\mlir\test\Examples\Toy\Ch7\jit.toy

Event Timeline

reames created this revision.Feb 19 2021, 12:14 PM
reames requested review of this revision.Feb 19 2021, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 12:14 PM

I don't think this is solving the problem in the right place -- this is a general computeKnownBits() issue, not specific to SCEV. See D93974 for an alternative approach, and the discussion there regarding how to avoid the ephemeral value issue.

reames added a comment.EditedFeb 19 2021, 12:49 PM

I don't think this is solving the problem in the right place -- this is a general computeKnownBits() issue, not specific to SCEV. See D93974 for an alternative approach, and the discussion there regarding how to avoid the ephemeral value issue.

Note: Deleted because I had a silly mistake in my framing, will comment again with a fixed comment.

(re-post of an earlier comment with mistakes fixed and more context added)

I don't think this is solving the problem in the right place -- this is a general computeKnownBits() issue, not specific to SCEV. See D93974 for an alternative approach, and the discussion there regarding how to avoid the ephemeral value issue.

These are definitely related, but not, I think, directly the same. I'd actually started with an approach very similar to that review before switching course. I went back and cleaned up what I had. That's now posted as D97099.

Right now, I see three related issues: 1) arguments should use the first instruction in the function as context, 2) non-instruction values (such as globals!) should use the first instruction in the function as context, and 3) we should not bail out on an assume which is the context instruction.

The linked patch tries to target (1) and (3). This patch specifically only targets (1) and and maybe (2). The choice not to include (3) was very deliberate to allow step wise progress. We do need a generic solution to (3), but it shouldn't block (1) or (2).

On (2), this patch does not handle that case. It could - and to be honest, I originally thought it did - but as shown by the test case for constant expressions it doesn't currently work out. I'll will note that (2) can not (in general) be done inside computeKnownBits. Why? Because we don't know the function context it's invoked in. (Though, we might be able to play some games with the analyzes as dom tree and assumption cache are tied to a single function scope.)

I believe we should move forward with either this patch or D97099 in the near term. Both are surgical improvements, and don't require solving the (fairly hard) item #3.

reames planned changes to this revision.Feb 22 2021, 9:18 AM

A common issue to all four alternative approaches has come up in the review for D97092. Marking this as Plan Changes until discussion converges and we decide what to do about the common issue.