PNaCl ABI: Promote illegal integer types

lib/Transforms/NaCl/PromoteIntegers.cpp

Index: lib/Transforms/NaCl/PromoteIntegers.cpp
diff --git a/lib/Transforms/NaCl/PromoteIntegers.cpp b/lib/Transforms/NaCl/PromoteIntegers.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..001a7de2fa43663c7168531eb8673b62642e9d55
--- /dev/null
+++ b/lib/Transforms/NaCl/PromoteIntegers.cpp
@@ -0,0 +1,527 @@
+//===- PromoteIntegers.cpp -  ---------------------------------------------===//

[Mark Seaborn, 2013/05/06 17:04:06]

Add a short description here?

[eliben, 2013/05/06 21:58:40]

IMHO the name "PromoteIntegers" is a bit too generic. It doesn't tell much and
there potentially can be several promotion passes with different goals.

How about something more detailed like "LegalizeIntegersForPNaCl" ? 

[Derek Schuff, 2013/05/08 22:33:28]

Done.

[Derek Schuff, 2013/05/08 22:33:28]

I LegalizeIntegersForPNaCl is even more generic; the name was chosen to reflect
the limited nature of the pass. I guess it depends on whether we want to split
other legalizations into another pass or add them to this pass.

Actually, I guess "legalize" is more generic, but "for pnacl" is more specific. 
Maybe call it PromoteIntegersForPNaCl, or just make it more generic and use
something like a TargetTransformInfo to parameterize it?

+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+// A limited set of transformations to promote illegal-sized int types.

[eliben, 2013/05/06 21:58:40]

Beef up this comment with a more detailed description of the type of
transformations the pass does (unless there's another place already when this is
described on a high level)

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+// It always maintains the invariant that the upper bits (above the size of the
+// original type) are zero.
+//
+//===----------------------------------------------------------------------===//
+//

[Mark Seaborn, 2013/05/06 17:04:06]

Add a longer description here

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/IRBuilder.h"
+#include "llvm/Pass.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace llvm;
+namespace {
+class PromoteIntegers : public FunctionPass {
+ public:
+  static char ID;
+  PromoteIntegers() : FunctionPass(ID) {
+    initializePromoteIntegersPass(*PassRegistry::getPassRegistry());
+  }
+  virtual bool runOnFunction(Function &F);
+};
+}
+
+char PromoteIntegers::ID = 0;
+INITIALIZE_PASS(PromoteIntegers, "nacl-promote-ints",
+                "Promote integer types which are illegal in PNaCl",
+                false, false);
+
+
+// Legal sizes are currently 1, 8, 16, 32, 64
+// We can't yet expand types above 64 bit, so don't try to touch them for now.
+static bool isLegalSize(unsigned Size) {

[Mark Seaborn, 2013/05/06 17:04:06]

Nit: as a static function (not a method), should this be named IsLegalSize()?

[Derek Schuff, 2013/05/08 22:33:28]

I don't think LLVM style distinguishes between static functions and methods for
naming. (at least not that i could find in the style guide)

[Mark Seaborn, 2013/05/09 01:00:22]

http://llvm.org/docs/CodingStandards.html matches actual practice in the
codebase for methods, but not for top-level functions, which mostly seem to
start with an upper case char.

e.g. NextPowerOf2() and others in include/llvm/Support/MathExtras.h.  However,
that file has counterexamples (e.g. isShiftedInt()).  LLVM isn't consistent,
unfortunately.

Not a big deal, but something I wondered about when preparing changes myself.
:-)

+  // TODO(dschuff): expand >64bit types or disallow >64bit packed bitfields
+  if (Size > 64) return true;
+  assert(Size <= 64 && "Don't know how to expand > 64bit types yet");

[Mark Seaborn, 2013/05/06 17:04:06]

This assert is redundant given the check above

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+  return isPowerOf2_32(Size) && !(Size & 0x6);

[Mark Seaborn, 2013/05/06 17:04:06]

It might be simpler just to list 1, 8, 16, 32, 64 here.

[eliben, 2013/05/06 21:58:40]

I agree

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+}
+
+// Return a legal integer or pointer-to-integer type, promoting to a larger
+// size if necessary.
+static Type *convertType(Type *Ty) {

[eliben, 2013/05/06 21:58:40]

PromoteIntegerType? In general, pick one verb consistently. You currently have
"convert", "promote" and "legalize" which AFAICS all refer to the same thing.


Also, would it be clearer to separate the pointer case into a function that uses
the integer one?

[Derek Schuff, 2013/05/08 22:33:28]

They are basically the same in this context. I renamed stuff to try to be a
little more clear. Now "convert" actually refers to making changes to the IR
(e.g. RAUW) or a new value of a constant, and convertType is now just
getPromotedType since it doesn't mutate things. The only references to
"legalize" are in comments, and are still correct in the sense that the
functions they describe don't actually care how the operands were legalized
(e.g. promoted, expanded, or already legal)

+  assert((isa<IntegerType>(Ty) || isa<PointerType>(Ty)) &&
+         "Trying to convert a non-integer type");
+  IntegerType *ITy;
+  if (isa<PointerType>(Ty))

[eliben, 2013/05/06 21:58:40]

Why are you handling pointers at all? I thought we don't plan having any
pointers in pexes?

[Derek Schuff, 2013/05/08 22:33:28]

The general strategy with these passes has been to make them generic enough not
to require every simplification, so the order can be more flexible. In this
case, support for pointers is pretty straightforward.

+    ITy = dyn_cast<IntegerType>(Ty->getContainedType(0));
+  else
+    ITy = dyn_cast<IntegerType>(Ty);
+
+  unsigned Width = ITy->getBitWidth();
+  assert(Width <= 64 && "Don't know how to legalize >64 bit types yet");
+  if (isLegalSize(Width))
+    return Ty;
+  ITy = IntegerType::get(Ty->getContext(), NextPowerOf2(Width));
+
+  if (isa<PointerType>(Ty))
+    return ITy->getPointerTo();
+  return ITy;
+}
+
+// Return true if Val is an int or pointer-to-int which should be converted.
+static bool shouldConvert(Value *Val) {
+  Type *Ty = Val->getType();
+  if (PointerType *Pty = dyn_cast<PointerType>(Ty))
+    Ty = Pty->getContainedType(0);
+  if (IntegerType *ITy = dyn_cast<IntegerType>(Ty)) {
+    if (!isLegalSize(ITy->getBitWidth())) {
+      return true;
+    }
+  }
+  return false;
+}
+
+// Return a constant which has been promoted to a legal size.
+static Value *convertConstant(Constant *C, bool SignExt=false) {
+  assert(shouldConvert(C));
+  ConstantInt *CInt = cast<ConstantInt>(C);
+  return ConstantInt::get(
+      convertType(cast<IntegerType>(CInt->getType())),
+      SignExt ? CInt->getSExtValue() : CInt->getZExtValue(),
+      /*isSigned=*/SignExt);
+}
+
+class ConversionState {

[eliben, 2013/05/06 21:58:40]

Document what this class does and what each of its members holds

[Derek Schuff, 2013/05/08 22:48:32]

Done.

+ public:
+  // Return the promoted value for Val. If val has not yet been converted,

[eliben, 2013/05/06 21:58:40]

s/val/Val/

Also, you may want to follow the LLVM style for comments?

[Derek Schuff, 2013/05/08 22:33:28]

do you mean just use doxygen? (i did add doxy comments for the public
interfaces). or something else?

[eliben, 2013/05/09 16:42:19]

Yeah, like \p Val for arguments, etc.

+  // return a placeholder, which will be converted later.
+  Value *getConverted(Value *Val) {
+    if (!shouldConvert(Val))
+        return Val;
+    assert(!isa<GlobalVariable>(Val) && "Can't convert GlobalVariables");
+    if (RewrittenMap.count(Val))
+      return RewrittenMap[Val];
+    Value *P;
+    // Directly convert constants.
+    if (Constant *C = dyn_cast<Constant>(Val))
+      P = convertConstant(C, /*SignExt=*/false);

[Mark Seaborn, 2013/05/06 17:04:06]

This shouldn't be inserted into Placeholders, surely?  It can be returned.

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+    else

[Mark Seaborn, 2013/05/06 17:04:06]

Maybe use {}s since using 'else' (not sure if style guides require that)

[Derek Schuff, 2013/05/08 22:33:28]

It doesn't, and I've seen it this way in LLVM, but I also really don't like it.
so, done.

+      // No converted value available yet, so create a placeholder.
+      P = new Argument(convertType(Val->getType()));

[eliben, 2013/05/06 21:58:40]

Why doesn't it have a parent? 

[Derek Schuff, 2013/05/08 22:33:28]

you mean, the Argument used as a placeholder? It doesn't actually matter,
Argument is just a Value with the appropriate type, which will be replaced
later. (Value itself is abstract)

[eliben, 2013/05/09 16:42:19]

I mean in terms of memory management. It's not immediately clear it's not a
memory leak.

+    RewrittenMap[Val] = P;
+    Placeholders[Val] = P;
+    return P;
+  }
+
+  // Replace the uses of From with To, replace the uses of any
+  // placeholders for From, and optionally give From's name to To.
+  // Also mark To for deletion.
+  void recordConverted(Value *From, Value *To, bool TakeName=true) {

[Mark Seaborn, 2013/05/06 17:04:06]

Use "Instruction *From" since that's what the function requires below

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+    ToErase.push_back(cast<Instruction>(From));
+    if (!shouldConvert(From)) {
+      From->replaceAllUsesWith(To);
+    } else {
+      if (Placeholders.count(From)) {
+        // Resolve placeholders.
+        Placeholders[From]->replaceAllUsesWith(To);
+        Placeholders.erase(From);
+      }
+      RewrittenMap[From] = To;
+    }
+    if (TakeName) {
+      To->takeName(From);
+    }
+  }
+  ~ConversionState() {
+    for (SmallVectorImpl<Instruction*>::iterator I = ToErase.begin(),
+             E = ToErase.end(); I != E; ++I)
+      (*I)->dropAllReferences();
+    for (SmallVectorImpl<Instruction*>::iterator I = ToErase.begin(),
+             E = ToErase.end(); I != E; ++I)
+      (*I)->eraseFromParent();
+  }
+ private:
+  DenseMap<Value *, Value *> RewrittenMap;
+  DenseMap<Value *, Value *> Placeholders;
+  SmallVector<Instruction*, 8> ToErase;
+};
+
+// Split an illegal load into multiple legal loads and return the resulting
+// promoted value. The size of the load is assumed to be a multiple of 8.
+static Value *splitLoad(LoadInst *Inst, ConversionState &State) {
+  assert(!Inst->isVolatile() && !Inst->isAtomic() &&

[eliben, 2013/05/06 21:58:40]

Do we disallow/hide volatile loads for PNaCl?

[Derek Schuff, 2013/05/08 22:33:28]

I'm not sure we've converged on that yet. but certainly they can't be split in
this way, although it seems unlikely that anything would generate volatile
operations on odd types like this.

+         "Can't split volatile/atomic loads");
+  assert(cast<IntegerType>(Inst->getType())->getBitWidth() % 8 == 0);
+
+  Value *OrigPtr = State.getConverted(Inst->getPointerOperand());
+  // OrigPtr is a placeholder in recursive calls, and so has no name
+  if (OrigPtr->getName().empty())
+    OrigPtr->setName(Inst->getPointerOperand()->getName());
+  unsigned Width = cast<IntegerType>(Inst->getType())->getBitWidth();
+  Type *NewType = convertType(Inst->getType());
+  unsigned LoWidth = Width;
+
+  while (!isLegalSize(LoWidth)) LoWidth -= 8;
+  IntegerType *LoType = IntegerType::get(Inst->getContext(), LoWidth);
+  IntegerType *HiType = IntegerType::get(Inst->getContext(), Width - LoWidth);
+  IRBuilder<> IRB(Inst->getParent(), Inst);
+
+  Value *BCLo = IRB.CreateBitCast(
+      OrigPtr,
+      LoType->getPointerTo(),
+      OrigPtr->getName() + ".loty");
+  Value *LoadLo = IRB.CreateAlignedLoad(
+      BCLo, Inst->getAlignment(), Inst->getName() + ".lo");
+  Value *LoExt = IRB.CreateZExt(LoadLo, NewType, LoadLo->getName() + ".ext");
+  Value *GEPHi = IRB.CreateConstGEP1_32(BCLo, 1, OrigPtr->getName() + ".hi");
+  Value *BCHi = IRB.CreateBitCast(
+        GEPHi,
+        HiType->getPointerTo(),
+        OrigPtr->getName() + ".hity");
+
+  Value *LoadHi = IRB.CreateLoad(BCHi, Inst->getName() + ".hi");
+  if (!isLegalSize(Width - LoWidth)) {
+    LoadHi = splitLoad(cast<LoadInst>(LoadHi), State);
+    // BCHi was still illegal, and has been replaced with a placeholder in the
+    // recursive call. Since it is redundant with BCLo in the recursive call,
+    // just splice it out entirely.
+    State.recordConverted(BCHi, GEPHi, /*TakeName=*/false);
+  }
+
+  Value *HiExt = IRB.CreateZExt(LoadHi, NewType, LoadHi->getName() + ".ext");
+  Value *HiShift = IRB.CreateShl(HiExt, LoWidth, HiExt->getName() + ".sh");
+  Value *Result = IRB.CreateOr(LoExt, HiShift);
+
+  State.recordConverted(Inst, Result);
+
+  return Result;
+}
+
+static Value *splitStore(StoreInst *Inst, ConversionState &State) {
+  assert(!Inst->isVolatile() && !Inst->isAtomic() &&
+         "Can't split volatile/atomic stores");
+  assert(cast<IntegerType>(Inst->getValueOperand()->getType())->getBitWidth() % 8 == 0);

[Mark Seaborn, 2013/05/06 17:04:06]

Line >80 chars

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+
+  Value *OrigPtr = State.getConverted(Inst->getPointerOperand());
+  // OrigPtr is now a placeholder in recursive calls, and so has no name
+  if (OrigPtr->getName().empty())
+    OrigPtr->setName(Inst->getPointerOperand()->getName());
+  Value *OrigVal = State.getConverted(Inst->getValueOperand());
+  unsigned Width = cast<IntegerType>(
+      Inst->getValueOperand()->getType())->getBitWidth();
+  unsigned LoWidth = Width;
+
+  while (!isLegalSize(LoWidth)) LoWidth -= 8;
+  IntegerType *LoType = IntegerType::get(Inst->getContext(), LoWidth);
+  IntegerType *HiType = IntegerType::get(Inst->getContext(), Width - LoWidth);
+  IRBuilder<> IRB(Inst->getParent(), Inst);
+
+  Value *BCLo = IRB.CreateBitCast(
+      OrigPtr,
+      LoType->getPointerTo(),
+      OrigPtr->getName() + ".loty");
+  Value *LoTrunc = IRB.CreateTrunc(
+      OrigVal, LoType, OrigVal->getName() + ".lo");
+  IRB.CreateAlignedStore(LoTrunc, BCLo, Inst->getAlignment());
+
+  Value *HiLShr = IRB.CreateLShr(
+      OrigVal, LoWidth, OrigVal->getName() + ".hi.sh");
+  Value *GEPHi = IRB.CreateConstGEP1_32(BCLo, 1, OrigPtr->getName() + ".hi");
+  Value *HiTrunc = IRB.CreateTrunc(
+      HiLShr, HiType, OrigVal->getName() + ".hi");
+  Value *BCHi = IRB.CreateBitCast(
+        GEPHi,
+        HiType->getPointerTo(),
+        OrigPtr->getName() + ".hity");
+
+  Value *StoreHi = IRB.CreateStore(HiTrunc, BCHi);
+
+  if (!isLegalSize(Width - LoWidth)) {
+    // HiTrunc is still illegal, and is redundant with the truncate in the
+    // recursive call, so just get rid of it.
+    State.recordConverted(HiTrunc, HiLShr, /*TakeName=*/false);
+    StoreHi = splitStore(cast<StoreInst>(StoreHi), State);
+    // BCHi was still illegal, and has been replaced with a placeholder in the
+    // recursive call. Since it is redundant with BCLo in the recursive call,
+    // just splice it out entirely.
+    State.recordConverted(BCHi, GEPHi, /*TakeName=*/false);
+  }
+  State.recordConverted(Inst, StoreHi, /*TakeName=*/false);
+  return StoreHi;
+}
+
+// Return a value with the bits of the operand above the size of the original
+// type cleared. The operand is assumed to have been legalized already.
+static Value *getClearUpper(Value *Operand, Type *OrigType,
+                            Instruction *InsertPt) {
+  // If the operand is a constant, it will have been created by
+  // ConversionState.getConverted, which zero-extends by default.
+  if (isa<Constant>(Operand))
+    return Operand;
+  return BinaryOperator::Create(
+      Instruction::And,
+      Operand,
+      ConstantInt::get(
+          convertType(OrigType),
+          APInt::getLowBitsSet(convertType(OrigType)->getIntegerBitWidth(),
+                               OrigType->getIntegerBitWidth())),
+      Operand->getName() + ".clear",
+      InsertPt);
+}
+
+// Return a value with the bits of the operand above the size of the original
+// type equal to the sign bit of the original operand. The new operand is
+// assumed to have been legalized already.
+// This is done by shifting the sign bit of the smaller value up to the MSB
+// position in the larger size, and then arithmetic-shifting it back down
+static Value *getSignExtend(Value *Operand, Value *OrigOperand,
+                            Instruction *InsertPt) {
+  // If OrigOperand was a constant, NewOperand will have been created by
+  // ConversionState.getConverted, which zero-extends by default. But that is
+  // wrong here, so replace it with a sign-extended constant.
+  if (Constant *C = dyn_cast<Constant>(OrigOperand))
+    return convertConstant(C, /*SignExt=*/true);
+  Type *OrigType = OrigOperand->getType();
+  ConstantInt *ShiftAmt = ConstantInt::getSigned(
+      cast<IntegerType>(convertType(OrigType)),
+      convertType(OrigType)->getIntegerBitWidth() -
+        OrigType->getIntegerBitWidth());
+  BinaryOperator *Shl = BinaryOperator::Create(
+      Instruction::Shl,
+      Operand,
+      ShiftAmt,
+      Operand->getName() + ".getsign",
+      InsertPt);
+  return BinaryOperator::Create(
+      Instruction::AShr,
+      Shl,
+      ShiftAmt,
+      Operand->getName() + ".signed",
+      InsertPt);
+}
+
+bool PromoteIntegers::runOnFunction(Function &F) {
+  ConversionState State;
+  // Don't support changing the function arguments. This should not be
+  // generated by clang.
+  for (Function::arg_iterator I = F.arg_begin(), E = F.arg_end(); I != E; ++I) {
+    Value *Arg = I;
+    if (shouldConvert(Arg)) {
+      errs() << "Function " << F.getName() << ": " << *Arg << "\n";
+      llvm_unreachable("Cannot convert function with illegal argument");

[eliben, 2013/05/06 21:58:40]

"Convert" is too generic.

"Function argument has illegal integer/pointer type"

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+    }
+  }
+
+  for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI) {
+    for (BasicBlock::iterator BBI = FI->begin(), BBE = FI->end(); BBI != BBE;) {
+      Instruction *Inst = BBI++;
+      bool ShouldConvert = shouldConvert(Inst);

[eliben, 2013/05/06 21:58:40]

Comment to clarify the |=...

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+      for (User::op_iterator OI = Inst->op_begin(), OE = Inst->op_end();
+           OI != OE; ++OI)
+        ShouldConvert |= shouldConvert(cast<Value>(OI));
+      if (!ShouldConvert)
+        continue;
+
+      if (SExtInst *Sext = dyn_cast<SExtInst>(Inst)) {

[Mark Seaborn, 2013/05/06 17:04:06]

Maybe move this if/else chain to a top-level function?

[Derek Schuff, 2013/05/08 22:48:32]

Done.

+        Value *Op = Sext->getOperand(0);
+        Value *NewInst = NULL;
+        if (shouldConvert(Op)) {
+          NewInst = getSignExtend(State.getConverted(Op), Op, Sext);
+        }
+        if (convertType(Op->getType()) != convertType(Sext->getType())) {

[eliben, 2013/05/06 21:58:40]

Comment to explain the logic here

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+          NewInst = CastInst::CreateSExtOrBitCast(
+              NewInst ? NewInst : State.getConverted(Op),
+              convertType(cast<IntegerType>(Sext->getType())),
+              Sext->getName() + ".sext", Sext);
+        }
+        if (shouldConvert(Sext)) {
+          NewInst = getClearUpper(NewInst, Sext->getType(), Sext);
+        }
+        assert(NewInst && "Failed to convert sign extension");
+        State.recordConverted(Sext, NewInst);
+      } else if (ZExtInst *Zext = dyn_cast<ZExtInst>(Inst)) {
+        Value *Op = Zext->getOperand(0);
+        Value *NewInst = NULL;
+        // TODO(dschuff): Some of these zexts could be no-ops.
+        if (shouldConvert(Op)) {
+          NewInst = getClearUpper(State.getConverted(Op),
+                                  Op->getType(),
+                                  Zext);
+        }
+        if (convertType(Op->getType()) != convertType(Zext->getType())) {
+          NewInst = CastInst::CreateZExtOrBitCast(
+              NewInst ? NewInst : State.getConverted(Op),
+              convertType(cast<IntegerType>(Zext->getType())),
+              "", Zext);
+        }
+        assert(NewInst);
+        State.recordConverted(Zext, NewInst);
+      } else if (TruncInst *Trunc = dyn_cast<TruncInst>(Inst)) {
+        Value *Op = Trunc->getOperand(0);
+        Value *NewInst = NULL;
+        if (convertType(Op->getType()) != convertType(Trunc->getType())) {
+          NewInst = new TruncInst(
+              State.getConverted(Op),
+              convertType(cast<IntegerType>(Trunc->getType())),
+              State.getConverted(Op)->getName() + ".trunc",
+              Trunc);
+        }
+        if (shouldConvert(Trunc)) {

[eliben, 2013/05/06 21:58:40]

Here and below, didn't you already establish ShouldConvert before this if...else
chain?

[Derek Schuff, 2013/05/08 22:33:28]

Either the result or the operand needs to be converted, but not necessarily
both. hopefully the comments are clearer now.

+          NewInst = getClearUpper(NewInst ? NewInst : Op,
+                                  Trunc->getType(),
+                                  Trunc);
+        }
+        assert(NewInst);
+        State.recordConverted(Trunc, NewInst);
+      } else if (AllocaInst *Alloc = dyn_cast<AllocaInst>(Inst)) {
+        // Don't handle arrays of illegal types, but we could handle an array
+        // with size specified as an illegal type, as unlikely as that seems.
+        assert(!(shouldConvert(Alloc) && Alloc->isArrayAllocation()));
+        AllocaInst *NewInst = new AllocaInst(
+            convertType(Alloc->getAllocatedType()),
+            State.getConverted(Alloc->getArraySize()),
+            "", Alloc);
+        NewInst->setAlignment(Alloc->getAlignment());
+        State.recordConverted(Alloc, NewInst);
+      } else if (BitCastInst *BCInst = dyn_cast<BitCastInst>(Inst)) {
+        // Only handle pointers. Ints can't be casted to/from other ints

[Mark Seaborn, 2013/05/06 17:04:06]

It looks like you're trying to expand out pointers to illegal int types, but you
don't need to do that.  It would be simpler not to.  These pointer types will
get stripped out by the later ReplacePtrsWithInts pass that I will be adding.

e.g. Rather than trying to remove i31*, load/stores of i31 can simply bitcast
from the i31* pointer as a first step.

[Derek Schuff, 2013/05/08 22:33:28]

It's really not that much more complex to also change the pointers. This is
really the only part that cares, and keeping this means that the output really
will be free of the illegal types, independently of stripping pointers.

[Mark Seaborn, 2013/05/09 01:00:22]

This seems to touch:
 * getPromotedType()
 * shouldConvert()
 * AllocaInst handling
 * BitCast handling
It's only free of illegal types for the narrow set of uses generated by Clang. 
As you say, if there's an i31 inside a struct type, that's not stripped.

Similarly, the pass expands out i31* but not i31**, AFAICS.

It seems like it would complicate the testing of the pass for it to be changing
more.

For now, you could just have the ABI verifier check that no instruction or
argument has an illegal int type.  The illegal int type check doesn't have to be
recursive.

I'll leave it up to you, though.

+        if (shouldConvert(BCInst) || shouldConvert(BCInst->getOperand(0))) {
+          BitCastInst *NewInst = new BitCastInst(
+              State.getConverted(BCInst->getOperand(0)),
+              convertType(BCInst->getDestTy()),
+              "", BCInst);
+          State.recordConverted(BCInst, NewInst);
+        }
+      } else if (LoadInst *Load = dyn_cast<LoadInst>(Inst)) {
+        if (shouldConvert(Load)) {
+          splitLoad(Load, State);
+        }
+      } else if (StoreInst *Store = dyn_cast<StoreInst>(Inst)) {
+        if (shouldConvert(Store->getValueOperand())) {
+          splitStore(Store, State);
+        }
+      } else if (CallInst *Call = dyn_cast<CallInst>(Inst)) {
+        llvm_unreachable("can't convert calls");

[eliben, 2013/05/06 21:58:40]

Is this temporary? If not, why the code after it?

+        for (unsigned I = 0; I < Call->getNumArgOperands(); ++I) {
+          Value *Arg = Call->getArgOperand(I);
+          Call->setArgOperand(I, State.getConverted(Arg));
+        }
+      } else if (BinaryOperator *Binop = dyn_cast<BinaryOperator>(Inst)) {
+        Value *NewInst = NULL;
+        if (Binop->getOpcode() == Instruction::AShr) {

[eliben, 2013/05/06 21:58:40]

Can't this be done in the same switch below?

[Derek Schuff, 2013/05/08 22:33:28]

No; AShr needs to sign-extend its operand before doing the actual operation, and
the switch below is for what happens after the operation. (it's just made a
little less clear by the fact that in this case the original operation can be
combined with sign-extending the operand, because of the way that sign-extension
is implemented)

+          // Combine the sign-extension with the actual AShr operation.
+          Value *Op = Binop->getOperand(0);
+          Value *ShiftAmount = NULL;
+          APInt SignShiftAmt = APInt(
+              convertType(Op->getType())->getIntegerBitWidth(),
+              convertType(Op->getType())->getIntegerBitWidth() -
+              Op->getType()->getIntegerBitWidth());
+          NewInst = BinaryOperator::Create(
+              Instruction::Shl,
+              State.getConverted(Op),
+              ConstantInt::get(convertType(Op->getType()), SignShiftAmt),
+              State.getConverted(Op)->getName() + ".getsign",
+              Binop);
+          if (ConstantInt *C = dyn_cast<ConstantInt>(
+                  State.getConverted(Binop->getOperand(1)))) {
+            ShiftAmount = ConstantInt::get(convertType(Op->getType()),
+                                           SignShiftAmt + C->getValue());
+          } else {
+            ShiftAmount = BinaryOperator::Create(
+                Instruction::Add,
+                State.getConverted(Binop->getOperand(1)),
+                ConstantInt::get(convertType(Binop->getOperand(1)->getType()),
+                                 SignShiftAmt),
+                State.getConverted(Op)->getName() + ".shamt", Binop);
+          }
+          NewInst = BinaryOperator::Create(
+              Instruction::AShr,
+              NewInst,
+              ShiftAmount,
+              Binop->getName() + ".result", Binop);
+        } else {
+          NewInst = BinaryOperator::Create(
+              Binop->getOpcode(),
+              NewInst ? NewInst : State.getConverted(Binop->getOperand(0)),
+              State.getConverted(Binop->getOperand(1)),
+              Binop->getName() + ".result", Binop);
+        }
+
+        switch (Binop->getOpcode()) {
+          case Instruction::And:
+          case Instruction::Or:
+          case Instruction::Xor:
+          case Instruction::LShr:
+            // These won't change the upper bits.
+            break;
+          case Instruction::Add:
+          case Instruction::Sub:
+          case Instruction::Shl:
+          case Instruction::AShr:
+            // These can change the upper bits, so clear them now.
+            NewInst = getClearUpper(NewInst, Binop->getType(), Binop);
+            break;
+
+          // We should not see FP operators here.
+          // We don't handle mul/div.
+          case Instruction::FAdd:
+          case Instruction::FSub:
+          case Instruction::Mul:
+          case Instruction::FMul:
+          case Instruction::UDiv:
+          case Instruction::SDiv:
+          case Instruction::FDiv:
+          case Instruction::URem:
+          case Instruction::SRem:
+          case Instruction::FRem:
+          case Instruction::BinaryOpsEnd:
+            errs() << *Inst << "\n";
+            llvm_unreachable("Cannot handle binary operator");
+            break;
+        }
+
+        State.recordConverted(Binop, NewInst);
+      } else if (ICmpInst *Cmp = dyn_cast<ICmpInst>(Inst)) {
+        Value *Op0, *Op1;

[eliben, 2013/05/06 21:58:40]

Comment

[Derek Schuff, 2013/05/08 22:33:28]

Done.

+        if (Cmp->isSigned()) {
+          Op0 = getSignExtend(State.getConverted(Cmp->getOperand(0)),
+                              Cmp->getOperand(0),
+                              Cmp);
+          Op1 = getSignExtend(State.getConverted(Cmp->getOperand(1)),
+                              Cmp->getOperand(1),
+                              Cmp);
+        } else {
+          Op0 = State.getConverted(Cmp->getOperand(0));
+          Op1 = State.getConverted(Cmp->getOperand(1));
+        }
+        ICmpInst *NewInst = new ICmpInst(
+            Cmp, Cmp->getPredicate(), Op0, Op1, "");
+        State.recordConverted(Cmp, NewInst);
+      } else if (SelectInst *Select = dyn_cast<SelectInst>(Inst)) {
+        SelectInst *NewInst = SelectInst::Create(
+            Select->getCondition(),
+            State.getConverted(Select->getTrueValue()),
+            State.getConverted(Select->getFalseValue()),
+            "", Select);
+        State.recordConverted(Select, NewInst);
+      } else if (PHINode *Phi = dyn_cast<PHINode>(Inst)) {
+        PHINode *NewPhi = PHINode::Create(
+            convertType(Phi->getType()),
+            Phi->getNumIncomingValues(),
+            "", Phi);
+        for (unsigned I = 0, E = Phi->getNumIncomingValues(); I < E; ++I) {
+          NewPhi->addIncoming(State.getConverted(Phi->getIncomingValue(I)),
+                              Phi->getIncomingBlock(I));
+        }
+        State.recordConverted(Phi, NewPhi);
+      } else {
+        errs() << *Inst<<"\n";
+        llvm_unreachable("unhandled instruction");
+      }
+    }
+  }
+
+  return true;
+}