CodegenLIR.branchIns() has unhandled path

VERIFIED FIXED

Status

Tamarin
Virtual Machine
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Rick Reitmaier, Assigned: Rick Reitmaier)

Tracking

unspecified
x86
Mac OS X
Bug Flags:
flashplayer-qrb +

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Currently CodegenLIR branchIns() contains the following logic...

                else {
                    // not taken - no code to emit.
                    // fixme - but what to return? caller wants to patch something.
                    AvmAssert(false);
                }
 
we need to add support for this case.
(Assignee)

Comment 1

9 years ago
From Ed...

That code path is trying to emit a conditional branch that we know is not taken.  So it cannot be turned into a jump, and later code will want to patch it.  Either we need to detect the constant condition earlier, and not emit the branch, or some other creative solution. i can think of several; one is to add support in all the assemblers to support a bool const as a valid "condition", but thats hacky and a lot of work.
(Assignee)

Updated

9 years ago
Assignee: nobody → rreitmai
Status: NEW → ASSIGNED

Comment 2

9 years ago
IMHO we can't release redux->central without this fixed, since code that hits this generates faulty code (right?)

Updated

9 years ago
Blocks: 469836
(Assignee)

Comment 3

9 years ago
How about returning 0 from branchIns() and then adding logic to support a null return from the method. 

Seems like a null-check in patchLater() and a couple of other places would do it.

Catching the constant condition up front and not making the branchIns() call seems like extra work that would be scattered across quite a few places.

Comment 4

9 years ago
seems like it would work.

another possibility is to loosen up LIR semantics a bit so that a constant 1 or 0 is a legal input to the branch opcodes.  then the backend or a later pass can take care of optimizing dead code.  too much work in the short term?
(Assignee)

Comment 5

9 years ago
re : 0 or 1 as legal input

Yes we could do this too, but it might make the semantics for isCmp/isCond a little tricky, since they are used for optimization finding.  Either that or we'd need to add cases to support constant values in there.

Probably a little more work than is necessary, since one could also argue that the front-end shouldn't ever generate a conditional jump for a non-conditional compare.
(Assignee)

Comment 6

9 years ago
Created attachment 360819 [details] [diff] [review]
ver1 - applies against redux change 1403
Attachment #360819 - Flags: review?(edwsmith)

Comment 7

9 years ago
(In reply to comment #5)

> Probably a little more work than is necessary, since one could also argue that
> the front-end shouldn't ever generate a conditional jump for a non-conditional
> compare.

One could argue that, but it's hard to guarantee given the way the various filters interact with each other.  Anyway, for later.

Updated

9 years ago
Attachment #360819 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 8

9 years ago
pushed :
716c2ebe127d: Fix 469162 - CodegenLIR branchIns (r+edwsmith) default tip
diff
browse
Rick Reitmaier <rreitmai@adobe.com> - Tue, 10 Feb 2009 12:55:14 -0800 - rev 1433
Fix 469162 - CodegenLIR branchIns (r+edwsmith)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: flashplayer-qrb+

Comment 9

8 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.