Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14569012:
PNaCl ABI: Promote illegal integer types (Closed)
|
Created:
7 years, 7 months ago by Derek Schuff Modified:
7 years, 7 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionPNaCl ABI: Promote illegal integer types
This pass (mostly) legalizes integer types by promoting them.
It has some limitations (e.g. it can't change function types)
but it is more than sufficient for what clang and SROA generate.
A more significant limitation of promotion is that packed
bitfields of size > 64 bits are still not handled. There are
none in our tests (other than callingconv_case_by_case which
doesn't require a stable pexe) but we will want to either
handle them by correctly expanding them, or find a better way
to error out.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3360
R=eliben@chromium.org, jvoung@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=a0efa09
Patch Set 1 #Patch Set 2 : use upper-clear invariant rather than sign-extend #
Total comments: 79
Patch Set 3 : #Patch Set 4 : move logic to convertInstruction #
Total comments: 36
Patch Set 5 : #Patch Set 6 : handle nsw/nuw #
Total comments: 6
Patch Set 7 : reviews #Patch Set 8 : remove ABI checker tests until after we enable the pass #
Messages
Total messages: 15 (0 generated)
Some initial comments... https://codereview.chromium.org/14569012/diff/2001/lib/Analysis/NaCl/PNaClABI... File lib/Analysis/NaCl/PNaClABITypeChecker.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABITypeChecker.cpp:56: //Valid = true; You'll need to undo this before committing since the new pass isn't enabled yet. (As an aside, I'd like to move the enabled-passes list into the LLVM branch repo to make changes like this easier.) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:1: //===- PromoteIntegers.cpp - ---------------------------------------------===// Add a short description here? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:13: // Add a longer description here https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:44: static bool isLegalSize(unsigned Size) { Nit: as a static function (not a method), should this be named IsLegalSize()? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:47: assert(Size <= 64 && "Don't know how to expand > 64bit types yet"); This assert is redundant given the check above https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:48: return isPowerOf2_32(Size) && !(Size & 0x6); It might be simpler just to list 1, 8, 16, 32, 64 here. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:109: P = convertConstant(C, /*SignExt=*/false); This shouldn't be inserted into Placeholders, surely? It can be returned. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:110: else Maybe use {}s since using 'else' (not sure if style guides require that) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:121: void recordConverted(Value *From, Value *To, bool TakeName=true) { Use "Instruction *From" since that's what the function requires below https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:205: assert(cast<IntegerType>(Inst->getValueOperand()->getType())->getBitWidth() % 8 == 0); Line >80 chars https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:327: if (SExtInst *Sext = dyn_cast<SExtInst>(Inst)) { Maybe move this if/else chain to a top-level function? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:389: // Only handle pointers. Ints can't be casted to/from other ints 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. https://codereview.chromium.org/14569012/diff/2001/test/NaCl/PNaClABI/types-f... File test/NaCl/PNaClABI/types-function.ll (right): https://codereview.chromium.org/14569012/diff/2001/test/NaCl/PNaClABI/types-f... test/NaCl/PNaClABI/types-function.ll:20: ; CHECK: Function types has instruction operand with disallowed type: i17* Also not committable yet... https://codereview.chromium.org/14569012/diff/2001/tools/lto/LTOCodeGenerator... File tools/lto/LTOCodeGenerator.cpp (right): https://codereview.chromium.org/14569012/diff/2001/tools/lto/LTOCodeGenerator... tools/lto/LTOCodeGenerator.cpp:111: return false; How is this change related to the new pass? Commit separately, perhaps?
https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:1: //===- PromoteIntegers.cpp - ---------------------------------------------===// 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" ? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:8: // A limited set of transformations to promote illegal-sized int types. 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) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:48: return isPowerOf2_32(Size) && !(Size & 0x6); On 2013/05/06 17:04:06, Mark Seaborn wrote: > It might be simpler just to list 1, 8, 16, 32, 64 here. I agree https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:53: static Type *convertType(Type *Ty) { 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? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:57: if (isa<PointerType>(Ty)) Why are you handling pointers at all? I thought we don't plan having any pointers in pexes? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:96: class ConversionState { Document what this class does and what each of its members holds https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:98: // Return the promoted value for Val. If val has not yet been converted, s/val/Val/ Also, you may want to follow the LLVM style for comments? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:112: P = new Argument(convertType(Val->getType())); Why doesn't it have a parent? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:154: assert(!Inst->isVolatile() && !Inst->isAtomic() && Do we disallow/hide volatile loads for PNaCl? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:313: llvm_unreachable("Cannot convert function with illegal argument"); "Convert" is too generic. "Function argument has illegal integer/pointer type" https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:320: bool ShouldConvert = shouldConvert(Inst); Comment to clarify the |=... https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:333: if (convertType(Op->getType()) != convertType(Sext->getType())) { Comment to explain the logic here https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:371: if (shouldConvert(Trunc)) { Here and below, didn't you already establish ShouldConvert before this if...else chain? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:406: llvm_unreachable("can't convert calls"); Is this temporary? If not, why the code after it? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:413: if (Binop->getOpcode() == Instruction::AShr) { Can't this be done in the same switch below? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:487: Value *Op0, *Op1; Comment
https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... File test/Transforms/NaCl/promote-integers.ll (right): https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:16: %a12 = sext i8 %a to i12 move the definition %a12 = ... closer to the uses? (after the call to @consume) Add a CHECK: for how %a12 gets defined now? https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:20: ; CHECK: %a12.getsign = shl i16 %a12, 4 Could add other test cases where the shift amount is different, e.g. if you had an i10, or i11 to begin with. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:39: define void @zext_to_illegal(i32 %a) { CHECK-NOT: for the AND? (or is there an AND)? https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:45: ; TODO(dschuff): make the ANDs no-ops? Put the TODO by the first AND? https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:76: ; CHECK: %a16 = trunc i32 %a24 to i16 same for sext? https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:99: %cmp = icmp slt i24 %shl, -2 Is it the case that with "ult", you will still end up with the "getsign", etc. ? (-2 is extended to an i32, so it's only fair if the variable also had as many bits). https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:122: ; CHECK: %sum.result = add i32 %a24, 16777214 CHECK-NEXT: or is there something in between? https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:143: %b = lshr i24 %a24, 20 nit: Also test with shift amt smaller than 16? (with this test case, the %b would be all zeroes, right) https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:215: %bc = bitcast i8* %a to i24* Is it also possible to handle bitcast i32* %a to i24% + a load? It would still be composed of an i16 load and an i8 load right?
https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:1: //===- PromoteIntegers.cpp - ---------------------------------------------===// On 2013/05/06 17:04:06, Mark Seaborn wrote: > Add a short description here? Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:1: //===- PromoteIntegers.cpp - ---------------------------------------------===// On 2013/05/06 21:58:40, eliben wrote: > 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. 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? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:8: // A limited set of transformations to promote illegal-sized int types. On 2013/05/06 21:58:40, eliben wrote: > 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) Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:13: // On 2013/05/06 17:04:06, Mark Seaborn wrote: > Add a longer description here Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:44: static bool isLegalSize(unsigned Size) { On 2013/05/06 17:04:06, Mark Seaborn wrote: > Nit: as a static function (not a method), should this be named IsLegalSize()? 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) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:47: assert(Size <= 64 && "Don't know how to expand > 64bit types yet"); On 2013/05/06 17:04:06, Mark Seaborn wrote: > This assert is redundant given the check above Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:48: return isPowerOf2_32(Size) && !(Size & 0x6); On 2013/05/06 21:58:40, eliben wrote: > On 2013/05/06 17:04:06, Mark Seaborn wrote: > > It might be simpler just to list 1, 8, 16, 32, 64 here. > > I agree Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:53: static Type *convertType(Type *Ty) { On 2013/05/06 21:58:40, eliben wrote: > 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? 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) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:57: if (isa<PointerType>(Ty)) On 2013/05/06 21:58:40, eliben wrote: > Why are you handling pointers at all? I thought we don't plan having any > pointers in pexes? 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. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:98: // Return the promoted value for Val. If val has not yet been converted, On 2013/05/06 21:58:40, eliben wrote: > s/val/Val/ > > Also, you may want to follow the LLVM style for comments? do you mean just use doxygen? (i did add doxy comments for the public interfaces). or something else? https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:109: P = convertConstant(C, /*SignExt=*/false); On 2013/05/06 17:04:06, Mark Seaborn wrote: > This shouldn't be inserted into Placeholders, surely? It can be returned. Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:110: else On 2013/05/06 17:04:06, Mark Seaborn wrote: > Maybe use {}s since using 'else' (not sure if style guides require that) It doesn't, and I've seen it this way in LLVM, but I also really don't like it. so, done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:112: P = new Argument(convertType(Val->getType())); On 2013/05/06 21:58:40, eliben wrote: > Why doesn't it have a parent? 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) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:121: void recordConverted(Value *From, Value *To, bool TakeName=true) { On 2013/05/06 17:04:06, Mark Seaborn wrote: > Use "Instruction *From" since that's what the function requires below Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:154: assert(!Inst->isVolatile() && !Inst->isAtomic() && On 2013/05/06 21:58:40, eliben wrote: > Do we disallow/hide volatile loads for PNaCl? 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. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:205: assert(cast<IntegerType>(Inst->getValueOperand()->getType())->getBitWidth() % 8 == 0); On 2013/05/06 17:04:06, Mark Seaborn wrote: > Line >80 chars Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:313: llvm_unreachable("Cannot convert function with illegal argument"); On 2013/05/06 21:58:40, eliben wrote: > "Convert" is too generic. > > "Function argument has illegal integer/pointer type" Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:320: bool ShouldConvert = shouldConvert(Inst); On 2013/05/06 21:58:40, eliben wrote: > Comment to clarify the |=... Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:333: if (convertType(Op->getType()) != convertType(Sext->getType())) { On 2013/05/06 21:58:40, eliben wrote: > Comment to explain the logic here Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:371: if (shouldConvert(Trunc)) { On 2013/05/06 21:58:40, eliben wrote: > Here and below, didn't you already establish ShouldConvert before this if...else > chain? Either the result or the operand needs to be converted, but not necessarily both. hopefully the comments are clearer now. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:389: // Only handle pointers. Ints can't be casted to/from other ints On 2013/05/06 17:04:06, Mark Seaborn wrote: > 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. 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. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:413: if (Binop->getOpcode() == Instruction::AShr) { On 2013/05/06 21:58:40, eliben wrote: > Can't this be done in the same switch below? 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) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:487: Value *Op0, *Op1; On 2013/05/06 21:58:40, eliben wrote: > Comment Done. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... File test/Transforms/NaCl/promote-integers.ll (right): https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:16: %a12 = sext i8 %a to i12 On 2013/05/07 01:22:11, jvoung (cr) wrote: > move the definition %a12 = ... closer to the uses? (after the call to @consume) > > Add a CHECK: for how %a12 gets defined now? Done. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:20: ; CHECK: %a12.getsign = shl i16 %a12, 4 On 2013/05/07 01:22:11, jvoung (cr) wrote: > Could add other test cases where the shift amount is different, e.g. if you had > an i10, or i11 to begin with. Done. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:39: define void @zext_to_illegal(i32 %a) { On 2013/05/07 01:22:11, jvoung (cr) wrote: > CHECK-NOT: for the AND? (or is there an AND)? Done. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:45: ; TODO(dschuff): make the ANDs no-ops? On 2013/05/07 01:22:11, jvoung (cr) wrote: > Put the TODO by the first AND? Done. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:76: ; CHECK: %a16 = trunc i32 %a24 to i16 On 2013/05/07 01:22:11, jvoung (cr) wrote: > same for sext? same what? https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:99: %cmp = icmp slt i24 %shl, -2 No, with unsigned and equality we don't sign-extend, and just do the compares as the larger type with the upper bits clear. Constants also get zero-extended rather than sign-extended. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:122: ; CHECK: %sum.result = add i32 %a24, 16777214 On 2013/05/07 01:22:11, jvoung (cr) wrote: > CHECK-NEXT: > > or is there something in between? Done. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:143: %b = lshr i24 %a24, 20 On 2013/05/07 01:22:11, jvoung (cr) wrote: > nit: Also test with shift amt smaller than 16? (with this test case, the %b > would be all zeroes, right) True, although it's just an artifact of the way I got the illegal value to start with, and doesn't affect the code generated for the shift at all. https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:215: %bc = bitcast i8* %a to i24* On 2013/05/07 01:22:11, jvoung (cr) wrote: > Is it also possible to handle bitcast i32* %a to i24% + a load? > It would still be composed of an i16 load and an i8 load right? Yeah, the bitcast is just to get from a legal type (i8*) to the illegal type without using an illegal type in a function parameter or global. whether the cast is from i32* or i8* or some struct (as it is with SROA) is irrelevant. https://codereview.chromium.org/14569012/diff/2001/tools/lto/LTOCodeGenerator... File tools/lto/LTOCodeGenerator.cpp (right): https://codereview.chromium.org/14569012/diff/2001/tools/lto/LTOCodeGenerator... tools/lto/LTOCodeGenerator.cpp:111: return false; oops, yes this just fixes a warning, split into a separate change. On 2013/05/06 17:04:06, Mark Seaborn wrote: > How is this change related to the new pass? Commit separately, perhaps?
https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:96: class ConversionState { On 2013/05/06 21:58:40, eliben wrote: > Document what this class does and what each of its members holds Done. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:327: if (SExtInst *Sext = dyn_cast<SExtInst>(Inst)) { On 2013/05/06 17:04:06, Mark Seaborn wrote: > Maybe move this if/else chain to a top-level function? Done.
https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... File test/Transforms/NaCl/promote-integers.ll (right): https://codereview.chromium.org/14569012/diff/2001/test/Transforms/NaCl/promo... test/Transforms/NaCl/promote-integers.ll:76: ; CHECK: %a16 = trunc i32 %a24 to i16 On 2013/05/08 22:33:28, Derek Schuff wrote: > On 2013/05/07 01:22:11, jvoung (cr) wrote: > > same for sext? > same what? meant test trunc with a var that was sext to illegal, but I guess it wouldn't be different. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:128: return convertConstant(C, /*SignExt=*/false); Is it possible to keep constants in RewrittenMap? I guess not because you later find that you want a sign extended version instead? https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:142: ToErase.push_back(cast<Instruction>(From)); do you still need a cast? https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:143: if (!shouldConvert(From)) { Haven't read through everything, yet, but it seems odd that it !shouldConvert(...), yet it will be replaced by To. Add comment? https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:150: } should there be an else { From->replaceAllUsesWith(To); } it seems like there is still a case where From won't be replaced by To. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:171: // Illegal values which have already been converted, to erase on distruction. nits: distruction -> destruction consistency with * spacing in "SmallVector<Instruction*, 8> ToErase", vs the above "Value *"
https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:44: static bool isLegalSize(unsigned Size) { On 2013/05/08 22:33:28, Derek Schuff wrote: > On 2013/05/06 17:04:06, Mark Seaborn wrote: > > Nit: as a static function (not a method), should this be named IsLegalSize()? > 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) 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. :-) https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:389: // Only handle pointers. Ints can't be casted to/from other ints On 2013/05/08 22:33:28, Derek Schuff wrote: > On 2013/05/06 17:04:06, Mark Seaborn wrote: > > 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. > > It's really not that much more complex to also change the > pointers. This is really the only part that cares, This seems to touch: * getPromotedType() * shouldConvert() * AllocaInst handling * BitCast handling > and keeping this means that the output really will be free of the > illegal types, independently of stripping pointers. 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. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:8: // A limited set of transformations to promote illegal-sized int types. Add //==---... spacer before this, to follow the style https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:12: // It always maintains the invariant that the upper bits (above the size of the I don't think that's the best choice of invariant, BTW, because only a subset of operations (div/rem, shr, cmp, switch), care about those upper bits. The rest don't (add, mul, sub, and, xor, shl, sext/zext, etc.). That would mean that if you're doing a series of adds, you'd be unnecessarily clearing the upper bits after each one. It would be better to clear the bits only before the operations that care. That's what I do in https://github.com/mseaborn/tiny-llvm-codegen. However, it was easy to test it comprehensively in that context. :-) What you're doing now is valid, and erring on the side of safety. Maybe it's worth an explanatory comment if you don't want to change it? https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:22: //===----------------------------------------------------------------------===// Follow the style here and end comment with: // //===----... https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:34: using namespace llvm; Add empty line after this https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:65: return IntegerType::get(Ty->getContext(), NextPowerOf2(Width)); NextPowerOf2() isn't right for i3, which should be promoted to i8, not i4. I don't think you're testing i3...i7 at the moment? https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:157: ~ConversionState() { The destructor should come before other methods. But I'd suggest not doing this side-effecting in a destructor. Explicit is better than implicit (except for classes specifically for RAII cleanup :-) ). https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:176: // promoted value. The size of the load is assumed to be a multiple of 8. Does this mean the pass doesn't handle i2...i7? https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:178: assert(!Inst->isVolatile() && !Inst->isAtomic() && This can get compiled out of release builds. Do a stricter check and use report_fatal_error()? assert() should be for internal consistency checks but not for validating external inputs. The same might apply to other asserts here. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:180: assert(cast<IntegerType>(Inst->getType())->getBitWidth() % 8 == 0); This should be a report_fatal_error() too since it's a check on the pass's input. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:229: assert(cast<IntegerType>(Inst->getValueOperand()->getType())->getBitWidth() % 8 Line >80 chars https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:269: State.recordConverted(cast<Instruction>(HiTrunc), HiLShr, /*TakeName=*/false); Line >80 chars https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:429: for (unsigned I = 0; I < Call->getNumArgOperands(); ++I) { Drop the dead code here since it's after llvm_unreachable()?
LGTM from me otherwise https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:98: // Return the promoted value for Val. If val has not yet been converted, On 2013/05/08 22:33:28, Derek Schuff wrote: > On 2013/05/06 21:58:40, eliben wrote: > > s/val/Val/ > > > > Also, you may want to follow the LLVM style for comments? > do you mean just use doxygen? (i did add doxy comments for the public > interfaces). or something else? Yeah, like \p Val for arguments, etc. https://codereview.chromium.org/14569012/diff/2001/lib/Transforms/NaCl/Promot... lib/Transforms/NaCl/PromoteIntegers.cpp:112: P = new Argument(convertType(Val->getType())); On 2013/05/08 22:33:28, Derek Schuff wrote: > On 2013/05/06 21:58:40, eliben wrote: > > Why doesn't it have a parent? > 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) I mean in terms of memory management. It's not immediately clear it's not a memory leak.
https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:8: // A limited set of transformations to promote illegal-sized int types. On 2013/05/09 01:00:22, Mark Seaborn wrote: > Add //==---... spacer before this, to follow the style Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:12: // It always maintains the invariant that the upper bits (above the size of the I did think about doing that. For the operations that are actually generated by clang and SROA (and/or/lshr/shl/trunc/zext), it didn't make much difference (I've never seen any arithmetic instructions actually generated). Instcombine can generated signed icmp which is bad either way, but that's a probably a pessimization even outside of pnacl, so i'll probably just leave that disabled. Also I just realized that all the shl operations have the nuw flag which means we can skip the clear for those. BTW I think add et al. do actually care about the extra upper bits. If there is no invariant in place, they could be anything, and could cause the operation in the promoted type to overflow and change the result in the lower bits. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:22: //===----------------------------------------------------------------------===// On 2013/05/09 01:00:22, Mark Seaborn wrote: > Follow the style here and end comment with: > // > //===----... Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:34: using namespace llvm; On 2013/05/09 01:00:22, Mark Seaborn wrote: > Add empty line after this Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:65: return IntegerType::get(Ty->getContext(), NextPowerOf2(Width)); On 2013/05/09 01:00:22, Mark Seaborn wrote: > NextPowerOf2() isn't right for i3, which should be promoted to i8, not i4. I > don't think you're testing i3...i7 at the moment? Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:128: return convertConstant(C, /*SignExt=*/false); On 2013/05/09 00:06:44, jvoung (cr) wrote: > Is it possible to keep constants in RewrittenMap? I guess not because you later > find that you want a sign extended version instead? It is possible because the clients that want sign extension get it by explicitly calling convertConstant and not using getConverted. The first version of this code did that, it doesn't really matter either way. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:142: ToErase.push_back(cast<Instruction>(From)); On 2013/05/09 00:06:44, jvoung (cr) wrote: > do you still need a cast? Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:143: if (!shouldConvert(From)) { On 2013/05/09 00:06:44, jvoung (cr) wrote: > Haven't read through everything, yet, but it seems odd that it > !shouldConvert(...), yet it will be replaced by To. Add comment? Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:150: } On 2013/05/09 00:06:44, jvoung (cr) wrote: > should there be an else { > From->replaceAllUsesWith(To); > } > > it seems like there is still a case where From won't be replaced by To. added comment https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:157: ~ConversionState() { On 2013/05/09 01:00:22, Mark Seaborn wrote: > The destructor should come before other methods. But I'd suggest not doing this > side-effecting in a destructor. Explicit is better than implicit (except for > classes specifically for RAII cleanup :-) ). Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:171: // Illegal values which have already been converted, to erase on distruction. On 2013/05/09 00:06:44, jvoung (cr) wrote: > nits: > > distruction -> destruction > > consistency with * spacing in "SmallVector<Instruction*, 8> ToErase", vs the > above "Value *" Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:176: // promoted value. The size of the load is assumed to be a multiple of 8. On 2013/05/09 01:00:22, Mark Seaborn wrote: > Does this mean the pass doesn't handle i2...i7? Not for loads and stores. The rest of the ops should handle them without any extra special casing. For e.g. loads it could round to the next 8 bits, load and then clear the upper bits, but it didn't seem necessary to put the extra logic in there. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:178: assert(!Inst->isVolatile() && !Inst->isAtomic() && On 2013/05/09 01:00:22, Mark Seaborn wrote: > This can get compiled out of release builds. Do a stricter check and use > report_fatal_error()? assert() should be for internal consistency checks but > not for validating external inputs. > > The same might apply to other asserts here. Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:180: assert(cast<IntegerType>(Inst->getType())->getBitWidth() % 8 == 0); On 2013/05/09 01:00:22, Mark Seaborn wrote: > This should be a report_fatal_error() too since it's a check on the pass's > input. Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:229: assert(cast<IntegerType>(Inst->getValueOperand()->getType())->getBitWidth() % 8 On 2013/05/09 01:00:22, Mark Seaborn wrote: > Line >80 chars Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:269: State.recordConverted(cast<Instruction>(HiTrunc), HiLShr, /*TakeName=*/false); On 2013/05/09 01:00:22, Mark Seaborn wrote: > Line >80 chars Done. https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:429: for (unsigned I = 0; I < Call->getNumArgOperands(); ++I) { On 2013/05/09 01:00:22, Mark Seaborn wrote: > Drop the dead code here since it's after llvm_unreachable()? Done.
https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:12: // It always maintains the invariant that the upper bits (above the size of the On 2013/05/09 18:30:24, Derek Schuff wrote: > BTW I think add et al. do actually care about the extra upper > bits. If there is no invariant in place, they could be > anything, and could cause the operation in the promoted type to > overflow and change the result in the lower bits. FWIW, that's not true. For addition, digit n in the input (counting from the least significant digit) can only influence digits >=n in the result. That follows from the algorithm for addition, which processes the digits from right to left.
also added nsw/nuw handling https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/16001/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:12: // It always maintains the invariant that the upper bits (above the size of the OK, true. In any case it doesn't matter for the instructions we actually see, so I think we can leave it this way for now, and only change it if there becomes a good reason to in the future.
LGTM https://codereview.chromium.org/14569012/diff/34002/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/34002/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:485: NewInst ? NewInst : State.getConverted(Binop->getOperand(0)), NewInst is always null entering this branch? https://codereview.chromium.org/14569012/diff/34002/test/Transforms/NaCl/prom... File test/Transforms/NaCl/promote-integers.ll (right): https://codereview.chromium.org/14569012/diff/34002/test/Transforms/NaCl/prom... test/Transforms/NaCl/promote-integers.ll:178: ; CHECK: %ashl = shl nuw i16 %a12, 5 CHECK-NOT: and if you are checking the clearUpperBits optimization? https://codereview.chromium.org/14569012/diff/34002/test/Transforms/NaCl/prom... test/Transforms/NaCl/promote-integers.ll:231: ; TODO(dschuff): implement icmp sXX This doesn't seem to be a TODO anymore, according to the code. You also have a test for icmp slt. If it is still a TODO, what case?
https://codereview.chromium.org/14569012/diff/34002/lib/Transforms/NaCl/Promo... File lib/Transforms/NaCl/PromoteIntegers.cpp (right): https://codereview.chromium.org/14569012/diff/34002/lib/Transforms/NaCl/Promo... lib/Transforms/NaCl/PromoteIntegers.cpp:485: NewInst ? NewInst : State.getConverted(Binop->getOperand(0)), On 2013/05/10 01:22:44, jvoung (cr) wrote: > NewInst is always null entering this branch? Done. https://codereview.chromium.org/14569012/diff/34002/test/Transforms/NaCl/prom... File test/Transforms/NaCl/promote-integers.ll (right): https://codereview.chromium.org/14569012/diff/34002/test/Transforms/NaCl/prom... test/Transforms/NaCl/promote-integers.ll:178: ; CHECK: %ashl = shl nuw i16 %a12, 5 On 2013/05/10 01:22:44, jvoung (cr) wrote: > CHECK-NOT: and > if you are checking the clearUpperBits optimization? Done. https://codereview.chromium.org/14569012/diff/34002/test/Transforms/NaCl/prom... test/Transforms/NaCl/promote-integers.ll:231: ; TODO(dschuff): implement icmp sXX On 2013/05/10 01:22:44, jvoung (cr) wrote: > This doesn't seem to be a TODO anymore, according to the code. You also have a > test for icmp slt. If it is still a TODO, what case? Done.
Message was sent while issue was closed.
Committed patchset #8 manually as ra0efa09 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
