This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: patch: relax VEC_SELECT assertion

On Oct 4, 2004, at 5:58 PM, Roger Sayle wrote:

On Mon, 4 Oct 2004, Stuart Hastings wrote:
In simplify-rtx.c(simplify_binary_operation), an assertion of
"VECTOR_MODE_P (GET_MODE (trueop0))" causes an unnecessary ICE.  This
patch relaxes the assertion into a "if (non-vector-mode) then return
0".  Given the returned zero, the caller (combiner) realizes the given
RTX is illegal, and it eventually figures out something else that will

This patch is working around the real problem. You need to figure out how an operand of non-vector type ended up as the first operand of the vec_select. A little digging reveals this instruction:

(insn 31 30 33 0 (set (reg:V8QI 59 [ D.2583 ])
        (eq:V8QI (reg:V8QI 60 [ D.2580 ])
            (reg:V8QI 60 [ D.2580 ]))) 627 {eqv8qi3}
    (expr_list:REG_EQUAL (const_int 1 [0x1])

which has an incorrect REG_EQUAL note.

I agree this REG_EQUAL is incorrect and should be fixed, but it's moot; here is an untested patch that removes the REG_EQUAL, yet the testcase still fails:

Attachment: cse.c.diffs.txt
Description: Text document

  The problem is that somewhere
in the RTL optimizers we're performing the transformation (eq X X) ->
const1_rtx.  Unfortunately, this is incorrect when the machine mode
of the EQ is a vector mode.

(eq X X) is a tautology, and is true regardless of mode; as "const1_rtx" seems to be RTL-ese for "TRUE," I think those RTL optimizers are correct. But I agree that the transformation done with this conclusion is wrong; that's why my patch prevented that transformation.

Employing your preferred semantics in an x86/MMX runtime context, (eq X X) in V8QImode should yield a 64-bit mask of all ones.

I'd guess that a real implementation of the runtime semantics would require several new target hooks and some non-trivial work in all the vector-capable GCC targets, something neither GCC nor I could stomach right now.

The two possible correct solutions are

I think you are confusing "correct" with "preferred" here; more below.

either to disable this transformation for VECTOR_MODE_P, or alternatively
construct the appropriate CONST_VECTOR for true and false in the
appropriate mode.

Now that I understand your mindset, I prefer the former. This patch fixes the testcase, but has not yet been bootstrapped or regression-tested; it defeats RTL simplifications of vector-mode comparisons, and it may be too conservative:

Attachment: cse.c.diffs.txt
Description: Text document

I appreciate that your patch resolves the ICE for your testcase, but
unfortunately your solution is simply papering over a far more significant
correctness problem in the compiler.

Well, if we can't agree that (eq X X) is a tautology, we have some serious correctness problems. :-)

I did not fix this in simplify-rtx.csimplify_binary_operation() because it was easy to "paper over" the problem there; I chose it because this function is defined to "Return 0 if no simplification is possible." The routine generally does simplifications when it recognizes certain cases, and otherwise do nothing....

...except when simplifying two certain vector operations. The function contains twenty assertions; of these, four near the beginning seem to check for valid inputs, six are specific to VEC_CONCAT, and ten are dedicated to VEC_SELECT. Believing that these VEC-specific assertions are outside the spirit of the original design (the function isn't intended to be a correctness checker), I decided the best solution was to have it quietly reject the invalid const_1_rtx/vector-instruction, without asserting, just like it would do for any other invalid combination in a non-vector context.

If you feel differently, that's O.K. with me; there are probably more than a dozen workable places to catch this. The invalid const_1_rtx/VEC_SELECT combination was randomly generated by combine_simplify_rtx(); here is a patch that invalidates such vector-mode comparison simplifications. It will need removing when your more ambitious solution (below) finally gets implemented. This hasn't yet been bootstrapped or regression-tested:

Attachment: combine.diff.txt
Description: Text document

I've even warned of this potential problem with vector equality operators
in the past:

I agree with your points there, that's clearly the Right Thing to do for the general case. Until it's done, can we figure out how to prevent GCC from ICE-ing on this testcase?

I'll offer the testcase as a separate patch; I don't think it needs to be a hostage to this discussion. :-)

stuart hastings
Apple Computer

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]