[PATCH] D73804: [GVN] Add GVNOption to control load-pre more fine-grained.

Mon Feb 3 23:11:56 PST 2020

fedor.sergeev added inline comments.

================
Comment at: llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll:1
+; RUN: opt < %s -basicaa -gvn -enable-load-in-loop-pre=false -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
----------------
hgreving wrote:
> fedor.sergeev wrote:
> > hgreving wrote:
> > > fedor.sergeev wrote:
> > > > It makes sense to add both positive - checking that load-pre is performed when enabled,  and negative - checking that load-pre does not happen when disabled
> > > > (say, to catch a case when load-pre is not performed due to some bug or change in other defaults).
> > > The positive case is checked in load-pre.ll. Does this suffice? WDYT?
> > Shouldnt then -enable-load-in-loop-pre=true be added explicitly to load-pre.ll, to avoid its behavior being dependant on default setting?
> I think it _should_ depend on the default setting, as this should be considered a switch that explicitly diverts from the default IMO. I _can_ add this to loop-pre.ll, do you really want this? Either way ok with me.
The point is that defaults do change.
And I do see some variation of load-pre-in-loop restriction being quite useful by default.
(see my earlier comment on possibly enabling this unconditionally).

So it is quite normal to have a test that explicitly tests both true and false, just to avoid a hassle of updating tests upon a toggle of default value.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73804/new/

https://reviews.llvm.org/D73804