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

Juergen Ributzka juergen at apple.com
Tue Nov 19 15:20:09 PST 2013


Hi Hal and Andy,

thanks for looking at this. I started to rewrite the patch to incorporate the changes you suggested, but the changed schedule has uncovered some other issues I need to resolve first to fix the existing unit test.

Cheers,
Juergen


On Nov 19, 2013, at 10:17 AM, Andrew Trick <atrick at apple.com> wrote:

> 
> 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/ea32839e/attachment.html>


More information about the llvm-commits mailing list