[PATCH] D18662: [GVN] Fix handling of sub-byte types in big-endian mode

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Very nice fix. Minor complaints below about variable naming, feel free to address and commit as you see fit.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:778
@@ -777,3 +777,5 @@
   if (DL.isBigEndian()) {
-    StoredVal = IRB.CreateLShr(StoredVal, StoreSize - LoadSize, "tmp");
   }
----------------
You know, "StoreSize" and "LoadSize" are really bad variable names since they aren't actually the type store size... Which makes much more sense out of how this bug got in there in the first place.

Maybe rename them (in a separate patch) to something more clearly to do with value sizes rather than store and load sizes?

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1061
@@ -1058,4 +1060,3 @@
     if (DL.isBigEndian())
-      RV = Builder.CreateLShr(RV,
-                    NewLoadSize*8-SrcVal->getType()->getPrimitiveSizeInBits());
+      RV = Builder.CreateLShr(RV, (NewLoadSize - SrcValSize) * 8);
     RV = Builder.CreateTrunc(RV, SrcVal->getType());
----------------
Here, we see the reverse confusion IMO: SrcValSize would be better named SrcValStoreSize.


http://reviews.llvm.org/D18662