While hacking around in the SelectionDAG build code, I've made the
isVolatile, (new) isNonTemporal and Alignment parameters to
SelectionDAG::getLoad/getStore and friends non-default.
I've already caught one bug in the XCore backend by doing this:
if (Offset % 4 == 0) {
// We've managed to infer better alignment information than the load
// already has. Use an aligned load.
return DAG.getLoad(getPointerTy(), dl, Chain, BasePtr, NULL, 4,
false, false, 0);
}
Whoops! There's no alignment info being set here!
Is there any reason to keep these as default arguments? It invites silent
coding errors. I did this because I have to propagate the non-temporal
information through various phases of lowering and making these non-default
helps me catch missing cases.
We've done the same in our local sources and we've never found
specifying the extra arguments to be burdensome.
-Dave
On Thursday 11 February 2010 17:40:24 David Greene wrote:> While hacking around in the SelectionDAG build code, I've made the > isVolatile, (new) isNonTemporal and Alignment parameters to > SelectionDAG::getLoad/getStore and friends non-default. > > I've already caught one bug in the XCore backend by doing this: > > if (Offset % 4 == 0) { > // We've managed to infer better alignment information than the load > // already has. Use an aligned load. > return DAG.getLoad(getPointerTy(), dl, Chain, BasePtr, NULL, 4, > false, false, 0); > } > > Whoops! There's no alignment info being set here! > > Is there any reason to keep these as default arguments? It invites silent > coding errors. I did this because I have to propagate the non-temporal > information through various phases of lowering and making these non-default > helps me catch missing cases. > > We've done the same in our local sources and we've never found > specifying the extra arguments to be burdensome.I have a patch ready to track nontemporal semantics but I'm waiting for an opinion on this. I'd really like to get this integrated for 2.7. In my opinion default arguments are a bad idea in this context. It's very easy to make coding errors. I found a couple more today. Thanks. -Dave
On Fri, Feb 12, 2010 at 9:43 PM, David Greene <dag at cray.com> wrote:> On Thursday 11 February 2010 17:40:24 David Greene wrote: >> While hacking around in the SelectionDAG build code, I've made the >> isVolatile, (new) isNonTemporal and Alignment parameters to >> SelectionDAG::getLoad/getStore and friends non-default. >> >> I've already caught one bug in the XCore backend by doing this: >> >> if (Offset % 4 == 0) { >> // We've managed to infer better alignment information than the load >> // already has. Use an aligned load. >> return DAG.getLoad(getPointerTy(), dl, Chain, BasePtr, NULL, 4, >> false, false, 0); >> } >> >> Whoops! There's no alignment info being set here! >> >> Is there any reason to keep these as default arguments? It invites silent >> coding errors. I did this because I have to propagate the non-temporal >> information through various phases of lowering and making these non-default >> helps me catch missing cases. >> >> We've done the same in our local sources and we've never found >> specifying the extra arguments to be burdensome. > > I have a patch ready to track nontemporal semantics but I'm waiting for an > opinion on this. I'd really like to get this integrated for 2.7. In my > opinion default arguments are a bad idea in this context. It's very easy > to make coding errors. I found a couple more today.We had a similar problem in the ARM backend. You get correct code, but suboptimal scheduling. deep
On Fri, Feb 12, 2010 at 1:43 PM, David Greene <dag at cray.com> wrote:> On Thursday 11 February 2010 17:40:24 David Greene wrote: >> While hacking around in the SelectionDAG build code, I've made the >> isVolatile, (new) isNonTemporal and Alignment parameters to >> SelectionDAG::getLoad/getStore and friends non-default. >> >> I've already caught one bug in the XCore backend by doing this: >> >> if (Offset % 4 == 0) { >> // We've managed to infer better alignment information than the load >> // already has. Use an aligned load. >> return DAG.getLoad(getPointerTy(), dl, Chain, BasePtr, NULL, 4, >> false, false, 0); >> } >> >> Whoops! There's no alignment info being set here! >> >> Is there any reason to keep these as default arguments? It invites silent >> coding errors. I did this because I have to propagate the non-temporal >> information through various phases of lowering and making these non-default >> helps me catch missing cases. >> >> We've done the same in our local sources and we've never found >> specifying the extra arguments to be burdensome. > > I have a patch ready to track nontemporal semantics but I'm waiting for an > opinion on this. I'd really like to get this integrated for 2.7. In my > opinion default arguments are a bad idea in this context. It's very easy > to make coding errors. I found a couple more today.FWIW, I'm happy with removing default arguments. I'm not happy with long strings of simply-typed arguments like "NULL, 4, false, false, 0" that don't give a reader any indication of what the arguments mean, but that's trickier to fix.