[llvm-commits] [llvm-gcc] CastFixes To Make llvm-convert.cpp nearly signless (Review & Commit)

[Chris Lattner]

On Dec 20, 2006, at 3:13 PM, Reid Spencer wrote:
> The attached patch removes all but one isSigned() calls from
> llvm-convert.cpp and otherwise makes it signless. This cleans up the
> casting patch committed yesterday and bases signedness only on
> TYPE_UNSIGNED(TREE_TYPE(tree)). The one exception can't be removed  
> until
> the signed and unsigned integer types are unified.

Woot.

> Although I've tested this extensively and it causes no regressions, it
> needs to be reviewed. In particular, in two places I have changed
> casts-to-bool into SetCondInst to prevent "trunc to bool" semantics
> which could break things.

Ok.

> Some things to note:
>
> 1. EmitAsScalarType was just another name for a CastToType and had
> exactly one use
>    so I just removed it and replaced its call with a CastToType call.

Ok.  However, please change this:

@@ -1314,9 +1366,11 @@
  Value *TreeToLLVM::EmitCOND_EXPR(tree exp) {
+  // Emit the conditional expression and trunc/bitcast to BoolTy
+  Value *Cond = Emit(COND_EXPR_COND(exp), 0);
+  Cond = new SetCondInst(Instruction::SetNE, Cond,
+                         Constant::getNullValue(Cond->getType()),  
"toBool",
+                         CurBB);
    tree Then = COND_EXPR_THEN(exp);
    tree Else = COND_EXPR_ELSE(exp);

... to only make the setne instruction if Cond->getType() !=  
Type::BoolTy.  It is common for the predicate to be bool, so we  
should avoid the extraneous instruction if so.

  Value *TreeToLLVM::EmitTRUTH_NOT_EXPR(tree exp) {
    Value *V = Emit(TREE_OPERAND(exp, 0), 0);
+  V = new SetCondInst(Instruction::SetNE, V,
+                      Constant::getNullValue(V->getType()),  
"toBool", CurBB);
    V = BinaryOperator::createNot(V, V->getName()+"not", CurBB);
+  return CastToUIntType(V, ConvertType(TREE_TYPE(exp)));
  }

Likewise.

Likewise in TreeToLLVM::EmitTruthOp.

> 2. I've added several CastToXXType functions near the beginning of the
> file to
>    assist with casting int<->int, fp<->fp, and any<->any. This just
> prevents a
>    little opcode selection logic from being sprinkled throughout the
> source.

Ok.  Please rename CastToIntType -> CastToSIntType to make it  
explicit it is signed.

In CastToFPType, if srcbits == dstbits, just return V.



> 3. Most of the changes to llvm-types.cpp is just wrapping long  
> lines. I
> noticed it
>    so figured I'd fix it.



> 4. I added a tree typed argument to HandleScalarArgument so the  
> signedness
>    can be derived. This isn't always used, but its the only  
> accurate way to get
>    the signedness in a couple cases where it is used. If 0 is  
> passed in, it
>    defaults to unsigned.

Are you sure?  The code looks like it handles null as signed:

===================================================================
--- gcc/llvm-convert.cpp	(revision 230)
+++ gcc/llvm-convert.cpp	(working copy)
@@ -214,10 +214,9 @@
          // the actual impls is a short or char.
          assert(ArgVal->getType()->isIntegral() && LLVMTy->isIntegral 
() &&
                 "Lowerings don't match?");
-        Instruction::CastOps opcode = CastInst::getCastOpcode(
-            ArgVal, ArgVal->getType()->isSigned(), LLVMTy, LLVMTy- 
 >isSigned());
-        ArgVal =
-          CastInst::create(opcode, ArgVal, LLVMTy, NameStack.back(),  
CurBB);
+        bool isSigned = type == 0 ? true : !TYPE_UNSIGNED(type);
+        ArgVal = CastInst::createIntegerCast(ArgVal, LLVMTy, isSigned,
+                                             NameStack.back(), CurBB);



> 5. In two places (1787 and 2516) I removed some casting. These were
> remnants from
>    the SHIFT patch. Since shift no longer cares about the sign of its
> operands,
>    there's no need to cast them to force a particular kind of shift.

Great.

> 6. The NOOPCastToType function is a misnomer. The casts it produces  
> are
> often
>    not BitCast (the only "noop" cast we have). It can do integer
> conversions and
>    Ptr<->Int as well. I couldn't think of a better name for this so I
> just left it.

The only place where you call NOOPCastToType without a bitcast  
operand is in the "@@ -2475,8 +2544,18 @@" hunk of the patch.  Why  
not just change that the call CastToType, and change the rest to not  
pass bitcast?

While you're at it, you could rename NOOPCastToType to  
BitCastToType.  You can change "CastToType(BitCast, ...)" calls to  
call BitCastToType instead.

Doing this will also eliminate the call to isNoopCast (the ctor for  
bitcast does the check), which makes it dead.

> 7. The rest of the changes are of two forms: (a) change a cast to  
> use a
> specific
>    cast opcode, or (b) change a cast to use an opcode derived from
>    CastInst::getCastOpcode and the signedness information from the gcc
> tree
>    available.

This code (EmitLV_ARRAY_REF):

@@ -3832,7 +3917,7 @@
        IndexVal->getType() != Type::UIntTy &&
        IndexVal->getType() != Type::LongTy &&
        IndexVal->getType() != Type::ULongTy)
-    IndexVal = CastToType(IndexVal, Type::LongTy);
+    IndexVal = CastToType(Instruction::SExt, IndexVal, Type::LongTy);

    // Check for variable sized array reference.
    if (TREE_CODE(TREE_TYPE(Array)) == ARRAY_TYPE) {

Is incorrect.  We need to do two things here: If 'Index' is  
TREE_UNSIGNED and smaller than intptr_t, the CFE should emit a zext  
to intptr_t.  This is because the 'gep always does sign extension'  
could would handle it wrong, and explicitly inserting a sext is also  
wrong.

If the index is not TREE_UNSIGNED, and it is not 32/64 bits,  
inserting a sext is right.


> One small step towards signlessness.

Nice.  Please make the minor changes above, the result will be  
immediately commitable.  Thanks Reid,

-Chris

p.s. there is no need to cc me on patches, I subscribe to llvm- 
commits :)