[cfe-commits] [pr12251][patch] Use i1* for boolean loads and stores

[Rafael Espíndola]

> the language ref currently says that it is undefined what gets stored in the
> other 7 bits when you write an i1.  That was just to give freedom to be
> efficient, but should be changed to say "target dependent" or something like
> that if you want to go this way.

Good point. I noticed the store had to be i1 for the i1 load to be
legal, but I missed this one.

> There are also some real problems.  First off, writing an i1 will only ever
> write a byte, but doesn't ppc or some platform like that use 32 bit
> booleans?

I know they exist, but not much more (I don't know even if they are
supported). There are two possible cases:

* They use any non zero value as true. This optimization doesn't apply
at all and we should use the current approach.

* Memory positions are guaranteed to have only 0 or 1 (or another
fixed value). I think the best is to change the LLVM language ref to
say that an i1 load is legal in an area that use a i32 store (or at
least target defined). That way we can use i32 stores and i1 loads.

> That aside, the other problem is that the upper 7 bits are currently always
> set to zero.  That's no good for platforms that want all 8 bits to be all
> zero or all one, so the logic would need to change for them (see below).
> Various places (the DAG combiner?) know about the top 7 bits being zero, so
> they would need to be tweaked too.

Which platforms do that? Now that I think of it, there is not a lot to
gain from using i1 stores. Maybe we should just keep i8 stores (or
i32) and i1 loads. Do you see a problem with that?

> OK, so where does the "setting the top bits to zero" happen?  When a store
> of i1 hits the type legalizer, the promoted value (usually i8) is stored
> instead.  The upper bits of the promoted value are undefined , so you may
> wonder why I say the store will zero them.  The reason is that the store
> is promoted to a trunc store, not an ordinary store, a trunc store of i8 to
> i1.  As this i1 memory type isn't an operand or return type of the store (it
> is hidden away in the store node somewhere) it goes flying through the type
> legalizer without anything happening to it, and hits LegalizeDAG.
>
> In LegalizeDAG it hits this code (starting at line 1225):
>
>      if (StWidth != StVT.getStoreSizeInBits()) {
>        // Promote to a byte-sized store with upper bits zero if not
>        // storing an integral number of bytes.  For example, promote
>        // TRUNCSTORE:i1 X -> TRUNCSTORE:i8 (and X, 1)
>        EVT NVT = EVT::getIntegerVT(*DAG.getContext(),
>                                    StVT.getStoreSizeInBits());
>        Tmp3 = DAG.getZeroExtendInReg(Tmp3, dl, StVT);
>        SDValue Result =
>          DAG.getTruncStore(Tmp1, dl, Tmp3, Tmp2, ST->getPointerInfo(),
>                            NVT, isVolatile, isNonTemporal, Alignment);
>        ReplaceNode(SDValue(Node, 0), Result);
>
> This is probably where it should be changed to look at getBooleanContents
> and
> either use zero bits or same-bits depending on what that says.
>
> In the case of a load the real logic also occurs in LegalizeDAG and needs to
> be
> adjusted too.
>
> Ciao, Duncan.

Thanks!

Rafael