[PATCH][DAG] Fix non-deterministic code generation

Tue Nov 19 10:17:03 PST 2013

On Nov 19, 2013, at 10:01 AM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
>> From: "Andrew Trick" <atrick at apple.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "Juergen Ributzka" <juergen at apple.com>, "llvm commits" <llvm-commits at cs.uiuc.edu>, "Bill Wendling"
>> <isanbard at gmail.com>
>> Sent: Tuesday, November 19, 2013 11:54:57 AM
>> Subject: Re: [PATCH][DAG] Fix non-deterministic code generation
>> 
>> 
>> 
>> 
>> On Nov 18, 2013, at 11:16 AM, Hal Finkel < hfinkel at anl.gov > wrote:
>> 
>> 
>> 
>> ----- Original Message -----
>> 
>> 
>> From: "Juergen Ributzka" < juergen at apple.com >
>> To: "Hal Finkel" < hfinkel at anl.gov >
>> Cc: "LLVM Commits" < llvm-commits at cs.uiuc.edu >, "Andrew Trick" <
>> atrick at apple.com >, "Bill Wendling"
>> < isanbard at gmail.com >
>> Sent: Monday, November 18, 2013 12:59:20 PM
>> Subject: Re: [PATCH][DAG] Fix non-deterministic code generation
>> 
>> 
>> SelectionDAGISel::LowerArguments() is called before the first call to
>> SelectionDAGBuilder::visit(). This is why I still see SDNodes with
>> IR Order 0 in the DAG.
>> 
>> Okay, thanks for looking into this. I'm okay with the change you've
>> proposed, but please also add a comment that we're initializing the
>> value to zero to make sure that nodes generated from
>> SelectionDAGISel::LowerArguments are assigned a source order of
>> zero.
>> 
>> 
>> Thanks for reviewing this Hal.
>> 
>> 
>> Failing to reset IROrder was definitely an oversight. I talked with
>> Juergen and we agreed that resetting to one makes a bit more sense
>> since we use IROrder==0 for scheduling nodes that don’t have a
>> corresponding SD node (it’s a special case).
> 
> SGTM. Just to clarify, we'll have:
> 
> NodeOrder == 0 : A special case for nodes without a SDNode

I would say “A special case for nodes with no valid IR Order” because I think it’s possible to create SD nodes without either providing an initialized IR order (SDLoc) or mapping them back to an existing SD node.

> NodeOrder == 1 : Nodes from SelectionDAGISel::LowerArguments
> NodeOrder > 1  : Everything else.

That is my understanding. Yes, we need a comment.

Juergen, I think you can checkin if you confirm this behavior and add comments.

Thanks!
-Andy

> (I just want there to be a comment about this somewhere)
> 
> -Hal
> 
>> 
>> 
>> -Andy
>> 
>> 
>> 
>> 
>> -Hal
>> 
>> 
>> 
>> 
>> 
>> -Juergen
>> 
>> 
>> On Nov 18, 2013, at 10:37 AM, Hal Finkel < hfinkel at anl.gov > wrote:
>> 
>> 
>> 
>> ----- Original Message -----
>> 
>> 
>> From: "Juergen Ributzka" < juergen at apple.com >
>> To: "Hal Finkel" < hfinkel at anl.gov >
>> Cc: "LLVM Commits" < llvm-commits at cs.uiuc.edu >
>> Sent: Monday, November 18, 2013 11:59:17 AM
>> Subject: Re: [PATCH][DAG] Fix non-deterministic code generation
>> 
>> 
>> True, the relative order is the same, but the code that sorts the
>> availability list (src_ls_rr_sort::operator()) is sensitive to base
>> 0.
>> 
>> 
>> // Prefer an ordering where the lower the non-zero order number, the
>> higher
>> // the preference.
>> if ((LOrder || ROrder) && LOrder != ROrder)
>> return LOrder != 0 && (LOrder < ROrder || ROrder == 0);
>> 
>> 
>> LOrder and/or ROrder can be 0 if the IR Order is 0 or if there is no
>> SDNode associated with the scheduling unit. Considering that 0 seems
>> to indicate a special case, reseting IR Order to 0 doesn’t seem to
>> be a good idea and maybe it should be reset to 1. Another solution
>> would be to change the above code to be insensitive to base 0. What
>> do you think?
>> 
>> I don't understand the rationale for the current behavior. :( Andy?
>> Bill?
>> 
>> FWIW, it looks like SelectionDAGBuilder::visit increments SDNodeOrder
>> before it calls SelectionDAGBuilder::visit(unsigned Opcode, ...), so
>> setting it to 0 is probably okay.
>> 
>> -Hal
>> 
>> 
>> 
>> 
>> 
>> In general resting IR base still seems to be a good for the reason
>> you mentioned before.
>> 
>> 
>> -Juergen
>> 
>> 
>> 
>> 
>> On Nov 17, 2013, at 5:09 AM, Hal Finkel < hfinkel at anl.gov > wrote:
>> 
>> 
>> 
>> ----- Original Message -----
>> 
>> 
>> From: "Juergen Ributzka" < juergen at apple.com >
>> To: "Hal Finkel" < hfinkel at anl.gov >
>> Cc: "LLVM Commits" < llvm-commits at cs.uiuc.edu >
>> Sent: Sunday, November 17, 2013 1:02:37 AM
>> Subject: Re: [PATCH][DAG] Fix non-deterministic code generation
>> 
>> Hi Hal,
>> 
>> the availability queue in the list scheduler returns the nodes in IR
>> order. If there is more than one node ready to schedule, then the
>> difference in the IR order will produce different schedules.
>> 
>> Maybe I'm missing something, but that does not seem to explain the
>> problem. The relative order of the nodes should be the same whether
>> the first node starts with 0 or 29348.
>> 
>> It seems like we should do this anyway (or make the counter 64 bits).
>> For one thing, on really large inputs, the current 32-bit counter
>> could wrap (which would certainly produce this problem). But you're
>> seeing some other issue here.
>> 
>> -Hal
>> 
>> 
>> 
>> 
>> -Juergen
>> 
>> On Nov 16, 2013, at 10:40 PM, Hal Finkel < hfinkel at anl.gov > wrote:
>> 
>> 
>> 
>> Juergen,
>> 
>> Do you happen to know what in CodeGen is sensitive to the absolute
>> value of the node order?
>> 
>> -Hal
>> 
>> ----- Original Message -----
>> 
>> 
>> From: "Juergen Ributzka" < juergen at apple.com >
>> To: "LLVM Commits" < llvm-commits at cs.uiuc.edu >
>> Sent: Saturday, November 16, 2013 11:04:29 PM
>> Subject: [PATCH][DAG] Fix non-deterministic code generation
>> 
>> 
>> 
>> Hi @ll,
>> 
>> this patch resets SDNodeOrder in the SelectionDAGBuilder before
>> processing the IR of a new function. This is required to obtain
>> deterministic code generation for a function regardless of its
>> location in the source file.
>> 
>> Cheers,
>> Juergen
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>> 
>> 
>> 
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>> 
>> 
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>> 
>> 
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131119/eb1f5f1e/attachment.html>