[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>
> 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

> 
> 
> —escha

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