[PATCH] Fix DAG fold that didn't check legal ops

[Hal Finkel]

----- Original Message -----
> From: "escha" <escha at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Owen Anderson" <owen at apple.com>, "chandlerc" <chandlerc at gmail.com>
> Sent: Monday, May 18, 2015 2:17:18 PM
> Subject: Re: [PATCH] Fix DAG fold that didn't check legal ops
> 
> 
> > On May 18, 2015, at 12:06 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> > 
> > ----- Original Message -----
> >> From: "escha" <escha at apple.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> >> Sent: Monday, May 18, 2015 5:52:43 AM
> >> Subject: Re: [PATCH] Fix DAG fold that didn't check legal ops
> >> 
> >> 
> >> On May 18, 2015, at 12:31 AM, Hal Finkel < hfinkel at anl.gov >
> >> wrote:
> >> 
> >> Hi Escha,
> >> 
> >> In the future, please post patches like this using phabricator.
> >> Although trivial, it is much easier to see the full context of the
> >> patch using the web interface (because, when uploading a patch,
> >> you
> >> upload with full context and let the web interface reduce it
> >> dynamically). Please see:
> >> http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
> >> 
> >> 
> >> 
> >> Okay; I’ll use Phab from now on.
> >> 
> >> + (!LegalOperations || TLI.isOperationLegal(ISD::AND, VT)) &&
> >> 
> >> I'd prefer that we use TLI.isOperationLegalOrCustom here;
> >> otherwise,
> >> LGTM.
> >> 
> >> 
> >> For me, this came up when I was trying to implement the following
> >> optimization: when legalizing i64 AND with a custom lowering, if
> >> the
> >> high half of the output is known to be zero (for whatever reason),
> >> convert it from (AND A, B) into (ZERO_EXTEND (AND (TRUNC A, TRUNC
> >> B))). This caused an infinite DAG loop, because the DAG combine
> >> here
> >> tried to make it back into an AND even though i64 AND was not
> >> legal.
> >> 
> >> 
> >> If I use “isOperationLegalOrCustom” here, the infinite loop comes
> >> back, because it believes i64 AND is okay, so it proceeds to
> >> re-create the original pattern and call back into the same
> >> legalization code.
> >> 
> >> 
> >> Is what I’m doing inherently wrong, or is LegalOrCustom not the
> >> right
> >> thing here?
> > 
> > What I'm going to say here may be somewhat controversial (so I've
> > cc'd Owen so he can disagree if he'd like)...
> > 
> > We currently have a problem with defining the intended uses of
> > isOperationLegal vs isOperationLegalOrCustom. The issue is
> > indicating what 'Custom' represents. We generally assume that
> > Expanded things are expensive (and creating Expanded nodes after
> > operation legalization can create cycles). Legal things are
> > assumed to be cheap (this is not always true, but seems generally
> > correct). Are Custom things expensive? It depends on why the node
> > is custom lowered. Custom lowering could just be some kind of
> > target-specific expansion method, and in most cases can defer to
> > the default expansion logic. However, the operation could be cheap
> > (and might even defer to the TableGen isel logic in most cases),
> > but the C++ code might be recognizing cases that are even cheaper.
> > In this latter case, we should be treating Custom just like Legal.
> > I've often run across these kinds of cases, and so I've developed
> > a dislike for isOperationLegal.
> > 
> > In DAGCombine currently, isOperationLegalOrCustom occurs around 37
> > times, and isOperationLegal occurs 54 times. For both
> > isOperationLegal and isOperationLegalOrCustom, the checks often
> > occur in combination with a check for LegalOperations. Neither is
> > the clear winner. However, I realize that this information does
> > not help you with your cycle prevention problem. Chandler, as I
> > recall, ran into a number of these issues when working on the new
> > shuffle lowering in the x86 backend. What we eventually figured
> > out was that we should not create anti-canonical patterns. There
> > are some alternatives:
> > 
> > 1. Do the transformation in *ISelDAGToDAG instead (which has the
> > drawback of losing the reuse of the generic simplification of the
> > result)
> > 
> > 2. Do the transform at the SDAG level, but produce target-specific
> > nodes (same disadvantage as (1), although perhaps to a lesser
> > extent)
> > 
> > 3. Add a new callback in TLI to control the problematic behavior
> > 
> > In some sense (3) is the best option, as we already have a number
> > of is*Free, is*Cheap, isShuffleMaskLegal, etc. in TLI for this
> > reason, but the ever-growing number of inconsistent and
> > hard-to-test callbacks is also not a good thing.
> > 
> > So, I'm fine with using isOperationLegal here for now, so long as a
> > comment is added explaining why; that is a pragmatic solution to
> > the present problem. However, we really should figure out what to
> > do about the underlying problem of the mismatch between the varied
> > uses of Custom lowering and the implied cost information
> > inconsistently used throughout the codebase.
> > 
> > Thanks again,
> > Hal
> 
> As a long-term solution, maybe the problem (like you said) is that
> we’ve overloaded Custom as a term? In our case, we’re using “Custom”
> to say “we have a custom expansion for i64 AND, and also, it is not
> at all legal.” But like you said, some architectures may use Custom
> in a different way, e.g. to convert to some target-specific node or
> the like, but for an operation that is effectively “legal” to some
> degree or another.
> 
> Perhaps we need a “custom and legal” option, which means “the DAG
> combiner can generate this node during combine2”, and a “custom and
> not legal” option, which means “the DAG combiner *cannot* generate
> this node during combine2”, or something of that sort? Perhaps
> “legal” as a term needs to be defined in this sort of way, in terms
> of what the combiner is and is not allowed to generate
> post-legalization.

Generically I agree, and we should definitely separate the "how" (legal vs. custom) from our estimate of cost. However, for preventing cycles, this case highlights exactly why this is tricky: The definition of "this node" in your paragraph above means here the same node type but with a different data type, but that's not always the case. Sometimes creating the same node with a different data type is fine (cycle free). And sometimes creating some other pattern of nodes that happens to simplify into the nodes that will trigger a cycle is also possible.

Not a completely general solution, but would handle these kinds of things, I'd eventually like to see this kind of DAGCombine code and target code generated by TableGen, and then we could check for potential cycles automatically at build time (I recall chatting with Owen about this last year).

 -Hal

> 
> Feel free to disregard, since I’m hardly an expert on the DAG
> internals.
> 
> —escha

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory