Fix handling of the volatile bit of loads/stores in PNaCl bitcode files.

Fixes so that the volatile bit is no longer put into the bitcode file,
since the volatile bit is not in the PNaCl ABI.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3610
R=jvoung@chromium.org

Review URL: https://codereview.chromium.org/21949006

5 files changed, 73 insertions, 37 deletions

diff --git a/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h b/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h
index b83bb0f95b..30ec3e4df0 100644
--- a/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h
+++ b/include/llvm/Bitcode/NaCl/NaClLLVMBitCodes.h
@@ -289,11 +289,14 @@ namespace naclbitc {
     FUNC_CODE_INST_LOAD        = 20, // PNaCl version 1:
                                      //    LOAD: [op, align, vol]
                                      // PNaCl version 2:
-                                     //    LOAD: [op, align, vol, ty]
+                                     //    LOAD: [op, align, ty]
     // 21 is unused.
     // 22 is unused.
     FUNC_CODE_INST_VAARG       = 23, // Not used in PNaCl.
-    FUNC_CODE_INST_STORE       = 24, // STORE: [ptr, val, align, vol]
+    FUNC_CODE_INST_STORE       = 24, // PNaCl version 1:
+                                     //    STORE: [ptr, val, align, vol]
+                                     // PNaCl version 2:
+                                     //    Store: [ptr, val, align]
     // 25 is unused.
     FUNC_CODE_INST_EXTRACTVAL  = 26, // Not used in PNaCl.
     FUNC_CODE_INST_INSERTVAL   = 27, // Not used in PNaCl.
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
index 33ad03a246..f9d479767b 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
@@ -1644,41 +1644,50 @@ bool NaClBitcodeReader::ParseFunctionBody(Function *F) {
     }
     case naclbitc::FUNC_CODE_INST_LOAD: {
       // PNaCl version 1: LOAD: [op, align, vol]
-      // PNaCl version 2: LOAD: [op, align, vol, ty]
+      // PNaCl version 2: LOAD: [op, align, ty]
       unsigned OpNum = 0;
       Value *Op;
-      if (popValue(Record, &OpNum, NextValueNo, &Op))
+      if (popValue(Record, &OpNum, NextValueNo, &Op) ||
+          Record.size() != 3)
         return Error("Invalid LOAD record");
       switch (GetPNaClVersion()) {
         case 1:
-          if (Record.size() != 3)
-            return Error("Invalid LOAD record");
+	  I = new LoadInst(Op, "", Record[OpNum+1], (1 << Record[OpNum]) >> 1);
           break;
         case 2: {
-          if (Record.size() != 4)
-            return Error("Invalid LOAD record");
           // Add pointer cast to op.
-          Type *T = getTypeByID(Record[3]);
+          Type *T = getTypeByID(Record[2]);
           if (T == 0)
             return Error("Invalid type for load instruction");
           Op = ConvertOpToType(Op, T->getPointerTo(), CurBB);
           if (Op == 0) return true;
+	  I = new LoadInst(Op, "", false, (1 << Record[OpNum]) >> 1);
 	  break;
         }
       }
-      I = new LoadInst(Op, "", Record[OpNum+1], (1 << Record[OpNum]) >> 1);
       break;
     }
-    case naclbitc::FUNC_CODE_INST_STORE: { // STORE: [ptr, val, align, vol]
+    case naclbitc::FUNC_CODE_INST_STORE: {
+      // PNaCl version 1: STORE: [ptr, val, align, vol]
+      // PNaCl version 2: STORE: [ptr, val, align]
       unsigned OpNum = 0;
       Value *Val, *Ptr;
       if (popValue(Record, &OpNum, NextValueNo, &Ptr) ||
-          popValue(Record, &OpNum, NextValueNo, &Val) ||
-          OpNum+2 != Record.size())
+          popValue(Record, &OpNum, NextValueNo, &Val))
         return Error("Invalid STORE record");
-      // Note: In version 1, the following statement is a noop.
-      Ptr = ConvertOpToType(Ptr, Val->getType()->getPointerTo(), CurBB);
-      I = new StoreInst(Val, Ptr, Record[OpNum+1], (1 << Record[OpNum]) >> 1);
+      switch (GetPNaClVersion()) {
+      case 1:
+	if (OpNum+2 != Record.size())
+	  return Error("Invalid STORE record");
+	I = new StoreInst(Val, Ptr, Record[OpNum+1], (1 << Record[OpNum]) >> 1);
+	break;
+      case 2:
+	if (OpNum+1 != Record.size())
+	  return Error("Invalid STORE record");
+	Ptr = ConvertOpToType(Ptr, Val->getType()->getPointerTo(), CurBB);
+	I = new StoreInst(Val, Ptr, false, (1 << Record[OpNum]) >> 1);
+	break;
+      }
       break;
     }
     case naclbitc::FUNC_CODE_INST_CALL: {
diff --git a/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp b/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
index 9152879ff1..f6b85108e6 100644
--- a/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
+++ b/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
@@ -1007,23 +1007,35 @@ static bool WriteInstruction(const Instruction &I, unsigned InstID,
     break;
   case Instruction::Load:
     // PNaCl Version 1: LOAD: [op, align, vol]
-    // PNaCl Version 2: LOAD: [op, align, vol, ty]
+    // PNaCl Version 2: LOAD: [op, align, ty]
     Code = naclbitc::FUNC_CODE_INST_LOAD;
     pushValue(I.getOperand(0), InstID, Vals, VE, Stream);
     AbbrevToUse = FUNCTION_INST_LOAD_ABBREV;
     Vals.push_back(Log2_32(cast<LoadInst>(I).getAlignment())+1);
-    Vals.push_back(cast<LoadInst>(I).isVolatile());
+    if (PNaClVersion == 1) {
+      // Note: Even though volatile values are not part of the ABI,
+      // we must add a value to the record for version 1, since the
+      // reader for version 1 has already been released.
+      Vals.push_back(0);
+    }
     if (PNaClVersion == 2) {
       Vals.push_back(VE.getTypeID(I.getType()));
     }
     break;
-  case Instruction::Store: // STORE: [ptr, val, align, vol]
+  case Instruction::Store:
+    // PNaCl version 1: STORE: [ptr, val, align, vol]
+    // PNaCl version 2: STORE: [ptr, val, align]
     Code = naclbitc::FUNC_CODE_INST_STORE;
     AbbrevToUse = FUNCTION_INST_STORE_ABBREV;
     pushValue(I.getOperand(1), InstID, Vals, VE, Stream);
     pushValue(I.getOperand(0), InstID, Vals, VE, Stream);
     Vals.push_back(Log2_32(cast<StoreInst>(I).getAlignment())+1);
-    Vals.push_back(cast<StoreInst>(I).isVolatile());
+    if (PNaClVersion == 1) {
+      // Note: Even though volatile values are not part of the ABI,
+      // we must add a value to the record for version 1, since the
+      // reader for version 1 has already been released.
+      Vals.push_back(0);
+    }
     break;
   case Instruction::Call: {
     const CallInst &CI = cast<CallInst>(I);
@@ -1244,7 +1256,13 @@ static void WriteBlockInfo(const NaClValueEnumerator &VE,
     Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::FUNC_CODE_INST_LOAD));
     Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 6)); // Ptr
     Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 4)); // Align
-    Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, 1)); // volatile
+    if (PNaClVersion == 1) {
+      // Note: Even though volatile values are not part of the ABI,
+      // we must add a value to the record for version 1, since the
+      // reader for version 1 has already been released. By using a constant,
+      // we at least avoid wasting space in the bitcode file.
+      Abbv->Add(NaClBitCodeAbbrevOp(0));
+    }
     if (PNaClVersion == 2) {
       Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 4)); // Typecast
     }
@@ -1323,7 +1341,13 @@ static void WriteBlockInfo(const NaClValueEnumerator &VE,
     Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 6)); // Ptr
     Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 6)); // Value
     Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 4)); // Align
-    Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, 1)); // volatile
+    if (PNaClVersion == 1) {
+      // Note: Even though volatile values are not part of the ABI,
+      // we must add a value to the record for version 1, since the
+      // reader for version 1 has already been released. By using a constant,
+      // we at least avoid wasting space in the bitcode file.
+      Abbv->Add(NaClBitCodeAbbrevOp(0));
+    }
     if (Stream.EmitBlockInfoAbbrev(naclbitc::FUNCTION_BLOCK_ID,
                                    Abbv) != FUNCTION_INST_STORE_ABBREV)
       llvm_unreachable("Unexpected abbrev ordering!");
diff --git a/test/NaCl/Bitcode/bitcast-elide.ll b/test/NaCl/Bitcode/bitcast-elide.ll
index 37c19fb94c..baacfde558 100644
--- a/test/NaCl/Bitcode/bitcast-elide.ll
+++ b/test/NaCl/Bitcode/bitcast-elide.ll
@@ -45,7 +45,7 @@ define void @SimpleLoad() {
 
 ; PF2:       <FUNCTION_BLOCK NumWords=2 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=8/>
 ; PF2-NEXT:  </FUNCTION_BLOCK>
 
@@ -86,7 +86,7 @@ define void @SimpleLoadAlloca() {
 ; PF2-NEXT:     <CONSTANTS_BLOCK
 ; PF2:          </CONSTANTS_BLOCK>
 ; PF2-NEXT:     <INST_ALLOCA op0=1 op1=3/>
-; PF2-NEXT:     <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
+; PF2-NEXT:     <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
 ; PF2-NEXT:     <INST_RET abbrevid=8/>
 ; PF2-NEXT:   </FUNCTION_BLOCK>
 
@@ -119,7 +119,7 @@ define i32* @NonsimpleLoad(i32 %i) {
 ; PF2:       <FUNCTION_BLOCK NumWords=6 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
 ; PF2-NEXT:    <INST_CAST abbrevid=7 op0=2 op1=1 op2=11/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=9 op0=2/>
 ; PF2:       </FUNCTION_BLOCK>
 
@@ -163,8 +163,8 @@ define i32 @TwoLoads(i32 %i) {
 
 ; PF2:       <FUNCTION_BLOCK NumWords=7 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0 op3=0/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=3 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=3 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_BINOP abbrevid=5 op0=2 op1=1 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=9 op0=1/>
 ; PF2:       </FUNCTION_BLOCK>
@@ -206,8 +206,8 @@ define i32 @TwoLoadOpt(i32 %i) {
 
 ; PF2:       <FUNCTION_BLOCK NumWords=7 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0 op3=0/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=3 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=3 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_BINOP abbrevid=5 op0=2 op1=1 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=9 op0=1/>
 ; PF2:       </FUNCTION_BLOCK>
@@ -240,6 +240,6 @@ define void @SimpleStore(i32 %i) {
 
 ; PF2:        <FUNCTION_BLOCK NumWords=5 BlockCodeSize=4>
 ; PF2-NEXT:     <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:     <INST_STORE abbrevid=12 op0=2 op1=1 op2=3 op3=0/>
+; PF2-NEXT:     <INST_STORE abbrevid=12 op0=2 op1=1 op2=3/>
 ; PF2-NEXT:     <INST_RET abbrevid=8/>
 ; PF2:        </FUNCTION_BLOCK>
diff --git a/test/NaCl/Bitcode/inttoptr-elide.ll b/test/NaCl/Bitcode/inttoptr-elide.ll
index 988e16ea7b..8d573c8ca5 100644
--- a/test/NaCl/Bitcode/inttoptr-elide.ll
+++ b/test/NaCl/Bitcode/inttoptr-elide.ll
@@ -43,7 +43,7 @@ define void @SimpleLoad(i32 %i) {
 
 ; PF2:       <FUNCTION_BLOCK NumWords=5 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=8/>
 ; PF2:       </FUNCTION_BLOCK>
 
@@ -78,7 +78,7 @@ define i32* @NonsimpleLoad(i32 %i) {
 ; PF2:       <FUNCTION_BLOCK NumWords=6 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
 ; PF2-NEXT:    <INST_CAST abbrevid=7 op0=1 op1=1 op2=10/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=9 op0=2/>
 ; PF2:       </FUNCTION_BLOCK>
 
@@ -125,8 +125,8 @@ define i32 @TwoLoads(i32 %i) {
 
 ; PF2:       <FUNCTION_BLOCK NumWords=7 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_BINOP abbrevid=5 op0=2 op1=1 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=9 op0=1/>
 ; PF2:      </FUNCTION_BLOCK>
@@ -170,8 +170,8 @@ define i32 @TwoLoadOpt(i32 %i) {
 
 ; PF2:       <FUNCTION_BLOCK NumWords=7 BlockCodeSize=4>
 ; PF2-NEXT:    <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0 op3=0/>
-; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0 op3=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=1 op1=3 op2=0/>
+; PF2-NEXT:    <INST_LOAD abbrevid=4 op0=2 op1=3 op2=0/>
 ; PF2-NEXT:    <INST_BINOP abbrevid=5 op0=2 op1=1 op2=0/>
 ; PF2-NEXT:    <INST_RET abbrevid=9 op0=1/>
 ; PF2:       </FUNCTION_BLOCK>
@@ -206,6 +206,6 @@ define void @SimpleStore(i32 %i) {
 
 ; PF2:      <FUNCTION_BLOCK NumWords=5 BlockCodeSize=4>
 ; PF2-NEXT:   <DECLAREBLOCKS op0=1/>
-; PF2-NEXT:   <INST_STORE abbrevid=12 op0=1 op1=1 op2=3 op3=0/>
+; PF2-NEXT:   <INST_STORE abbrevid=12 op0=1 op1=1 op2=3/>
 ; PF2-NEXT:   <INST_RET abbrevid=8/>
 ; PF2T:     </FUNCTION_BLOCK>