On 08/31/2015 05:30 AM, Richard Biener wrote:
On Mon, Aug 31, 2015 at 7:49 AM, Mikhail Maltsev <malts...@gmail.com> wrote:
Hi, all!
This patch removes some conditional compilation from GCC. In this patch I define
a macro CHECKING_P, which is equal to 1 when ENABLE_CHECKING is defined and 0
otherwise. The reason for using a new name is the following: currently in GCC
there are many places where ENABLE_CHECKING is checked using #ifdef, and it is a
common way to do such checks (though #if would also work and is used in several
places). If we change it in such way that it is always defined, accidentally
using "#ifdef" instead of "#if" will lead to subtle errors: some expensive
checks intended only for development stage will be enabled in release build and
cause performance degradation.
This patch removes all uses of ENABLE_CHECKING (I also tried poisoning this
identifier in system.h, and the build succeeded, but I don't know how will this
affect, e.g. merging feature branches, so I think such decisions should be made
by maintainers).
I think we want to keep ENABLE_CHECKING for macro use and for some selected
cases.
Can you outline which cases you want to keep? My general feeling is to
avoid conditionally compiled code as much as we can.
As for conditional compilation, I tried to remove it and replace #ifdef-s with
if-s where possible, but, of course, avoided changing data structures layout. I
also avoided reindenting large blocks of code. I changed some functions (and a
couple of global variables) that were only used in "checking" build so that now
they are always defined/compiled and have a DEBUG_FUNCTION (i.e. "used")
attribute. I'll try to handle them in a more clean way: move CHECKING_P check
inside their definitions - that will slightly reduce "visual noise" from
conditions like
We'll eventually want to do the reindentations as well, but for an RFC,
that's fine.
While working on this patch I noticed some issues worth mentioning. In
sese.h:bb_in_region we have a check (enabled only in "checking" build):
/* Check that there are no edges coming in the region: all the
predecessors of EXIT are dominated by ENTRY. */
FOR_EACH_EDGE (e, ei, exit->preds)
dominated_by_p (CDI_DOMINATORS, e->src, entry);
IIUC, dominated_by_p has no side effects, so it's useless. Changing it to
"gcc_assert (dominated_by_p (...));" causes regressions. I disabled it in the
patch and added a comment.
You should open a bugreport.
Agreed. Looks like a simple bug.
In lra.c we have:
#ifdef ENABLE_CHECKING
/* Function checks RTL for correctness. If FINAL_P is true, it is
done at the end of LRA and the check is more rigorous. */
static void
check_rtl (bool final_p)
...
#ifdef ENABLED_CHECKING
extract_constrain_insn (insn);
#endif
...
#endif /* #ifdef ENABLE_CHECKING */
The comment for extract_constrain_insn says:
/* Do uncached extract_insn, constrain_operands and complain about failures.
This should be used when extracting a pre-existing constrained instruction
if the caller wants to know which alternative was chosen. */
So, as I understand, this additional check can be useful. This patch removes
"#ifdef ENABLED_CHECKING" (without regressions).
No strong opinions here. There's other things that would catch this
later. The check merely catches it closer to the point where things go
wrong. So I'd tend to want it to be conditional.
The third issue is that some gcc_checking_assert-s were guarded by #ifdef like:
#ifdef ENABLE_CHECKING
gcc_checking_assert (!is_deleted (*slot));
#endif
(is_deleted is defined unconditionally).
Probably that is because it is not obvious from the "release" definition
#define gcc_checking_assert(EXPR) ((void)(0 && (EXPR)))
that EXPR is not evaluated.
At least, I first decided to change it to something like
"do { if (0) (void)(EXPR); } while(0)"
and only then realized that the effect of "0 &&" is exactly the same. I added a
comment to make it more obvious.
Sounds reasonable.
I tested the patch on x86_64-pc-linux-gnu with --enable-checking=yes and
"release". Likewise, built config-list.mk in both configurations. There we some
minor changes since that check, but I'll need to rebase the patch anyway, so I
will repeat the full check.
Please review the patch.
Erm. Ok so if you are here... I've long wanted the heavier parts of checking
(like IL checking) to be controlled by a command-line switch (-fchecking or
even -fchecking[=...]) rather than being compile-time controlled. Obviously
that's not sth we want (or easily can get) for things like tree-checking or
asserts.
Just a suggestion which might eventually remove a whole bunch of
ENABLE_CHECKING #ifs before tackling the rest.
Seems like a reasonable thing to me.
jeff
