469162 - CodegenLIR.branchIns() has unhandled path

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Rick Reitmaier]

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.

Comment 1

[Rick Reitmaier]

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.

Comment 2

[Steven Johnson]

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

Comment 3

[Rick Reitmaier]

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

[Edwin Smith]

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?

Comment 5

[Rick Reitmaier]

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.

Comment 6

[Rick Reitmaier]

Attachment: ver1 - applies against redux change 1403

Comment 7

[Edwin Smith]

(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.

Comment 8

[Rick Reitmaier]

pushed :
716c2ebe127d: Fix 469162 - CodegenLIR branchIns (r+edwsmith) default tip
diff
browse
Rick Reitmaier <[e-mail]> - Tue, 10 Feb 2009 12:55:14 -0800 - rev 1433
Fix 469162 - CodegenLIR branchIns (r+edwsmith)

Comment 9

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.