On Mon, Jun 30, 2025 at 10:51:38AM -0400, Jason Merrill wrote: > > > So, kill the optimization unless idx is INTEGER_CST (am not sure > > > TREE_SIDE_EFFECTS is sufficient because it could be e.g. reading > > > from some > > > VAR_DECL), > > > > !TREE_SIDE_EFFECTS && tree_invariant_p? > > Or perhaps introduce a predicate that checks whether it's safe to use an > expression in both branches of a COND_EXPR, i.e. if there are no > SAVE_EXPR/TARGET_EXPR/BIND_EXPR?
That might work too. > > > and perhaps try to recover the optimization in GIMPLE? > > > Though, I think we don't gimplify ARRAY_REF into ARRAY_REF if the first > > > operand is not an array and don't create ARRAY_REFs for what has been > > > written as *(arr + idx). On the other side, ARRAY_REF is beneficial > > > primary for the simple cases, so maybe if op1 and op2 are ADDR_EXPRs > > > of non-volatile VAR_DECLs or COMPONENT_REFs with VAR_DECL bases so > > > constant or constant offset from frame, E1 is sequenced before E2 won't > > > be really observable and we could use the 3 SAVE_EXPRs. > > > > > > I think it has been added for > > > https://gcc.gnu.org/pipermail/gcc-patches/2001-April/thread.html#48160 > > > but I'm sure GIMPLE in between FE and expansion changed things > > > sufficiently > > > that it won't ICE anymore. > > > > And my comment in that thread was that pushing the indexing down into > > the COND_EXPR is probably undesirable once we start doing "significant > > optimization on trees" so maybe we should just drop it? > > I see that simply removing the COND_EXPR case from cp_build_array_ref brings > back the ICE from that thread, so more work is needed. > > I guess your suggestion of only pushing the indexing down for simple indexes > makes sense, otherwise do the *(a ? &b : &c)[d] transformation. For now I've bootstrapped/regtested on x86_64-linux and i686-linux the following version, which 1) for idx being INTEGER_CST (case for which I believe the change has been added) it keeps doing what it did 2) if both op1 and op2 are arrays with invariant address or pointers to something with invariant address, it uses SAVE_EXPR <op0>, SAVE_EXPR <idx> SAVE_EXPR <op0> as COND_EXPR condition and otherwise what it did 3) otherwise punt Now, perhaps a predicate (but how to call it) whether idx doesn't contain any SAVE_EXPRs/TARGET_EXPRs/BIND_EXPRs and isn't really large expression (but how to measure it; perhaps also no CALL_EXPRs, and the question if it should allow say multiplication/division/modulo, those might need larger code as well) could be used instead of the INTEGER_CST check, either immediately or incrementally. But guess I should first try to reproduce the ICE you've mentioned. > > For release branches, would unshare_expr (idx) for the second arm be > > enough? > > Ah, no, that doesn't help with SAVE_EXPR. Neither with TARGET_EXPRs I think, for neither of those I think unshare_expr actually unshares them and for a good reason, it can't know where is the SAVE_EXPR or TARGET_EXPR first used, whether inside of the expression, in that case it might be desirable to unshare those, or if in some earlier code, then it can't be unshared. 2025-06-30 Jakub Jelinek <ja...@redhat.com> PR c++/120471 * typeck.cc (cp_build_array_ref) <case COND_EXPR>: If idx is not INTEGER_CST, don't optimize the case or use SAVE_EXPR <op0>, SAVE_EXPR <idx>, SAVE_EXPR <op0> as new op0 depending on flag_strong_eval_order and whether op1 and op2 are arrays with invariant address or tree invariant pointers. Formatting fixes. * g++.dg/ubsan/pr120471.C: New test. --- gcc/cp/typeck.cc.jj 2025-06-30 10:59:28.016583215 +0200 +++ gcc/cp/typeck.cc 2025-06-30 11:49:54.879741494 +0200 @@ -4001,13 +4001,67 @@ cp_build_array_ref (location_t loc, tree } case COND_EXPR: - ret = build_conditional_expr - (loc, TREE_OPERAND (array, 0), - cp_build_array_ref (loc, TREE_OPERAND (array, 1), idx, - complain), - cp_build_array_ref (loc, TREE_OPERAND (array, 2), idx, - complain), - complain); + tree op0, op1, op2; + op0 = TREE_OPERAND (array, 0); + op1 = TREE_OPERAND (array, 1); + op2 = TREE_OPERAND (array, 1); + if (TREE_CODE (idx) != INTEGER_CST) + { + /* If idx could possibly have some SAVE_EXPRs, turning + (op0 ? op1 : op2)[idx] into + op0 ? op1[idx] : op2[idx] can lead into temporaries + initialized in one conditional path and uninitialized + uses of them in the other path. + And if idx is a really large expression, evaluating it + twice is also not optimal. + On the other side, op0 must be sequenced before evaluation + of op1 and op2 and for C++17 op0, op1 and op2 must be + sequenced before idx. + If idx is INTEGER_CST, we can just do the optimization + without any SAVE_EXPRs, if op1 and op2 are both ARRAY_TYPE + VAR_DECLs or COMPONENT_REFs thereof (so their address + is constant or relative to frame), optimize into + (SAVE_EXPR <op0>, SAVE_EXPR <idx>, SAVE_EXPR <op0>) + ? op1[SAVE_EXPR <idx>] : op2[SAVE_EXPR <idx>] + Otherwise avoid this optimization. */ + if (flag_strong_eval_order == 2) + { + if (TREE_CODE (TREE_TYPE (op1)) == ARRAY_TYPE) + { + tree xop1 = op1; + while (TREE_CODE (xop1) == COMPONENT_REF) + xop1 = TREE_OPERAND (xop1, 0); + STRIP_ANY_LOCATION_WRAPPER (xop1); + if (!decl_address_invariant_p (xop1)) + break; + } + else if (!POINTER_TYPE_P (TREE_TYPE (op1)) + || !tree_invariant_p (op1)) + break; + if (TREE_CODE (TREE_TYPE (op2)) == ARRAY_TYPE) + { + tree xop2 = op2; + while (TREE_CODE (xop2) == COMPONENT_REF) + xop2 = TREE_OPERAND (xop2, 0); + STRIP_ANY_LOCATION_WRAPPER (xop2); + if (!decl_address_invariant_p (xop2)) + break; + } + else if (!POINTER_TYPE_P (TREE_TYPE (op2)) + || !tree_invariant_p (op2)) + break; + } + if (TREE_SIDE_EFFECTS (idx)) + { + idx = save_expr (idx); + op0 = save_expr (op0); + tree tem = build_compound_expr (loc, op0, idx); + op0 = build_compound_expr (loc, tem, op0); + } + } + op1 = cp_build_array_ref (loc, op1, idx, complain); + op2 = cp_build_array_ref (loc, op2, idx, complain); + ret = build_conditional_expr (loc, op0, op1, op2, complain); protected_set_expr_location (ret, loc); return ret; --- gcc/testsuite/g++.dg/ubsan/pr120471.C.jj 2025-06-30 11:22:51.391566018 +0200 +++ gcc/testsuite/g++.dg/ubsan/pr120471.C 2025-06-30 11:22:51.391566018 +0200 @@ -0,0 +1,21 @@ +// PR c++/120471 +// { dg-do run } +// { dg-options "-fsanitize=undefined" } + +volatile int b[1], a[1]; + +void +foo (int x) +{ + volatile int c = 21; + volatile int v = (x % 2 ? b : a)[c % 3]; + if (v != 0) + __builtin_abort (); +} + +int +main () +{ + foo (1); + foo (2); +} Jakub
