Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
992493002:
Lower signatures exposing struct registers to byval struct pointers (Closed)
|
Created:
5 years, 9 months ago by Mircea Trofin Modified:
5 years, 9 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionLower signatures exposing struct registers to byval struct pointers
The new phase, NormalizeStructRegSignatures, converts signatures having parameters, or returning struct registers; or using structs that transitively reference such function types.
The phase relies on a subsequent phase to clean up the redundant alloca/load/store instructions, however, I'm still investigating which such phase should be.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3857
R=dschuff@chromium.org, jfb@chromium.org, mseaborn@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=6fbcecb7791690603fe502d3469cf6b639499e37
Patch Set 1 : First draft #
Total comments: 22
Patch Set 2 : Incorporated feedback #Patch Set 3 : determinism in output, more tests #
Total comments: 69
Patch Set 4 : Partial updates #Patch Set 5 : clang-format -ed #
Total comments: 22
Patch Set 6 : Simpler type mapping #Patch Set 7 : Incorporated more feedback #Patch Set 8 : Using IRBuilder #
Total comments: 13
Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : Debug Info for Functions #
Total comments: 12
Patch Set 12 : debug info #
Total comments: 2
Patch Set 13 : Tests for invoke, array, vector, vararg #
Total comments: 16
Patch Set 14 : landing pads #
Total comments: 13
Patch Set 15 : Varargs take 2 #
Total comments: 4
Patch Set 16 : Final. #Messages
Total messages: 30 (5 generated)
First quick review, some comments apply to later parts of the code. Also, please run clang-format on the file so it follows LLVM. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... File lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:35: #include <stddef.h> <cstddef> is more common. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:60: #include "llvm/Transforms/NaCl.h" You should sort these. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:64: class MappingResult { This should be in an anonymous namespace. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:118: bool ensurePNaClComplyingFunction(Function *OldFunc, Module &M); I'd avoid putting "PNaCl" in the name: when we upstream we'll have to drop it. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:125: const unsigned int TypicalArity = 8; static https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:149: FunctionType *OldFnTy = dyn_cast<FunctionType>(Ty); The LLVM-idiomatic way is: if (FunctionType *OldFnTy = dyn_cast<FunctionType>(Ty)) { https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:210: Type* &Loc = MappedTypes[StructTy]; Pointer ref is pretty unusual. Could you instead get a pointer, and insert into MappedTypes later? https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:233: SmallVector<Type*, 8> ElemTypes; TypicalArity? https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:235: for (unsigned I = 0; I < Ty->getStructNumElements(); I++) { I'd cache getStructNumElements. It's not expensive when doing LTO, but otherwise it's a call on each iteration. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:244: // correctly delete it Missing period https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:254: Type* &Loc = MappedTypes[StructTy]; Ditto on ptr ref. https://codereview.chromium.org/992493002/diff/1/test/Transforms/NaCl/normali... File test/Transforms/NaCl/normalize-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/1/test/Transforms/NaCl/normali... test/Transforms/NaCl/normalize-struct-reg-signatures.ll:13: define void @main(%struct* byval %ptr) { Could you move the CHECK next to each function? It's much easier to read that way.
https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... File lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:35: #include <stddef.h> On 2015/03/08 22:04:38, JF wrote: > <cstddef> is more common. Acknowledged. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:60: #include "llvm/Transforms/NaCl.h" On 2015/03/08 22:04:38, JF wrote: > You should sort these. Done. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:64: class MappingResult { On 2015/03/08 22:04:38, JF wrote: > This should be in an anonymous namespace. Done. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:118: bool ensurePNaClComplyingFunction(Function *OldFunc, Module &M); On 2015/03/08 22:04:38, JF wrote: > I'd avoid putting "PNaCl" in the name: when we upstream we'll have to drop it. Done. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:125: const unsigned int TypicalArity = 8; On 2015/03/08 22:04:38, JF wrote: > static Done. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:149: FunctionType *OldFnTy = dyn_cast<FunctionType>(Ty); On 2015/03/08 22:04:38, JF wrote: > The LLVM-idiomatic way is: > if (FunctionType *OldFnTy = dyn_cast<FunctionType>(Ty)) { Done. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:210: Type* &Loc = MappedTypes[StructTy]; On 2015/03/08 22:04:38, JF wrote: > Pointer ref is pretty unusual. Could you instead get a pointer, and insert into > MappedTypes later? Yes, but then we spend twice on the hash map location lookup. I'm fine doing the usual pattern, if we feel it outweighs the efficiency aspects. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:233: SmallVector<Type*, 8> ElemTypes; On 2015/03/08 22:04:38, JF wrote: > TypicalArity? Only I meant TypicalArity to be for functions :) But, I can do the same for structs, I think that's the right thing here. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:235: for (unsigned I = 0; I < Ty->getStructNumElements(); I++) { On 2015/03/08 22:04:38, JF wrote: > I'd cache getStructNumElements. It's not expensive when doing LTO, but otherwise > it's a call on each iteration. Done. https://codereview.chromium.org/992493002/diff/1/lib/Transforms/NaCl/Normaliz... lib/Transforms/NaCl/NormalizeStructRegSignatures.cpp:254: Type* &Loc = MappedTypes[StructTy]; On 2015/03/08 22:04:38, JF wrote: > Ditto on ptr ref. Ack, but see the comment before.
Deeper review, I still haven't gone through all the code, and test. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:2: // pointers That's a bit weird formatting :-) It would fit if you drop "Change"? https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:130: return MappedTypes[Ty]; auto Found = MappedTypes.find(Ty); if (Found != MappedTypes.end()) return *Found; https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:134: // transforms any type that could transitively reference a function pointer "Transforms" https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:138: assert(MappedTypes.end() == MappedTypes.find(Ty)); Is this an invariant? External code accesses this function from getSimpleType so it'll always check the map to make sure the types hasn't been rewritten yet, but I can't convince myself that recursive calls keep the invariant (and I think they should). https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:144: bool HasChanges = false; "Changed" is a more common name in LLVM. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:153: OldParam != E; ++OldParam) { for (auto P : OldFnTy->params()) https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:158: Type *NewFuncType = FunctionType::get(NewRetType, NewArgs, false); This should preserve vararg (and have a corresponding test). https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:159: return MappingResult(NewFuncType, HasChanges); C++11 initializer lists should work here and a few places below: s/return MappingResult(\(.*\));/return {\1};/g https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:165: return MappingResult(NewTy->getPointerTo(), NewTy.isChanged()); This should also preserve the address space of the pointer. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:177: NewTy.isChanged()); There's no test for this. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:189: Type *&Loc = MappedTypes[StructTy]; I'd use `find` here too. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:194: Loc = StructType::create(Ctx, StructTy->getStructName()); This loses `isPacked`, though I'm not sure we care? https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:202: // wgetStructNumElementsill differ. Paste-o? https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:209: return MappingResult(Loc, hasChanged); Same as above, just "Changed". https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:214: bool HasChanges = false; Changed https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:233: Type *&Loc = MappedTypes[StructTy]; `find` https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:235: StructType *NewStruct = dyn_cast<StructType>(Loc); Just `cast<StructType>`, which has an assert internally. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:241: // anything else stays the same. Capitalize (here and below). https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:271: for (const Argument &OldArg : OldArgList) { Move `OldFunc->getArgumentList()` here, delete above. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:289: for (const Argument &OldArg : OldFuncArgs) { Same, move `NewFunc->args()` here. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:300: // correct the types separately Where is the type corrected? It's not obvious from the surrounding lines and below usage of BlindReplace. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:302: for (auto UseIter = Old->use_begin(), E = Old->use_end(); E != UseIter;) { `for (auto U : Old->uses())` Actually, it would be better to use the approach RAUW has of `while (!use_empty())`. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:334: LastBlock != BIter;) { for (BasicBlock &BB : NewFunc->getBasicBlockList()) https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:336: for (auto IIter = BB->begin(), LastI = BB->end(); LastI != IIter;) { for (Instruction &I : BB) https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:339: auto &Ctx = Ret->getContext(); This should be loop invariant, hoist it. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:342: Ret->eraseFromParent(); I think this invalidates your Instruction iterator. You should create a worklist first, and then replace+delete (or create a kill list). https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:344: new StoreInst(RetVal, FirstNewArg, NewRet); Give it a name and copy debug location. Also, you can guarantee preferred alignment if you also do the same in the alloca. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:356: NewCall->setDebugLoc(Orig->getDebugLoc()); Other metadata? https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:362: Orig->getUnwindDest(), Args); Copy name. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:370: CallInst *Ret = CallInst::Create(Target, Args); Name. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:373: CopyCallAttributesAndMetadata(Orig, Ret); I think you're missing NoBuiltin, NoInline, ReturnTwice, NotAccessMemory, OnlyReadsMemory, NotReturn, NotThrow, CannotDuplicate. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:388: FunctionType *NewType = dyn_cast<FunctionType>( cast<FunctionType> https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:400: AllocaInst *Alloca = new AllocaInst(OldRetType); Alignment to preferred size. Name. You can also pass OldCall instead of doing insertBefore below. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:405: (Instruction *)nullptr); Alignment. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:408: } Why not create the new call between the alloca and the load? You'd be constructing in a more intuitive order, and could use a straight IRBuilder.
https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:2: // pointers On 2015/03/12 18:36:18, JF wrote: > That's a bit weird formatting :-) > > It would fit if you drop "Change"? Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:130: return MappedTypes[Ty]; On 2015/03/12 18:36:18, JF wrote: > auto Found = MappedTypes.find(Ty); > if (Found != MappedTypes.end()) > return *Found; or the location alternative, to avoid the double search :) https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:134: // transforms any type that could transitively reference a function pointer On 2015/03/12 18:36:18, JF wrote: > "Transforms" Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:138: On 2015/03/12 18:36:18, JF wrote: > assert(MappedTypes.end() == MappedTypes.find(Ty)); > > Is this an invariant? External code accesses this function from getSimpleType so > it'll always check the map to make sure the types hasn't been rewritten yet, but > I can't convince myself that recursive calls keep the invariant (and I think > they should). They don't. Struct mapping will insert the empty new struct there. Also we may hit memoized types when following pointers, for example. Alternatives would include passing a working map around, but I'm not sure how much more maintainable that is. I'll mark a TODO to find a simpler (more maintainable) algorithm, if possible, as a separate task. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:144: bool HasChanges = false; On 2015/03/12 18:36:18, JF wrote: > "Changed" is a more common name in LLVM. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:153: OldParam != E; ++OldParam) { On 2015/03/12 18:36:17, JF wrote: > for (auto P : OldFnTy->params()) Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:158: Type *NewFuncType = FunctionType::get(NewRetType, NewArgs, false); On 2015/03/12 18:36:17, JF wrote: > This should preserve vararg (and have a corresponding test). Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:159: return MappingResult(NewFuncType, HasChanges); On 2015/03/12 18:36:17, JF wrote: > C++11 initializer lists should work here and a few places below: > s/return MappingResult(\(.*\));/return {\1};/g Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:165: return MappingResult(NewTy->getPointerTo(), NewTy.isChanged()); On 2015/03/12 18:36:17, JF wrote: > This should also preserve the address space of the pointer. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:189: Type *&Loc = MappedTypes[StructTy]; On 2015/03/12 18:36:17, JF wrote: > I'd use `find` here too. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:194: Loc = StructType::create(Ctx, StructTy->getStructName()); On 2015/03/12 18:36:16, JF wrote: > This loses `isPacked`, though I'm not sure we care? No, we're setting that further below, if the new struct "wins". see line 230 https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:194: Loc = StructType::create(Ctx, StructTy->getStructName()); On 2015/03/12 18:36:16, JF wrote: > This loses `isPacked`, though I'm not sure we care? It doesn't, we set it below, line 229 https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:202: // wgetStructNumElementsill differ. On 2015/03/12 18:36:17, JF wrote: > Paste-o? yup https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:214: bool HasChanges = false; On 2015/03/12 18:36:17, JF wrote: > Changed Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:233: Type *&Loc = MappedTypes[StructTy]; On 2015/03/12 18:36:18, JF wrote: > `find` Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:235: StructType *NewStruct = dyn_cast<StructType>(Loc); On 2015/03/12 18:36:18, JF wrote: > Just `cast<StructType>`, which has an assert internally. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:271: for (const Argument &OldArg : OldArgList) { On 2015/03/12 18:36:18, JF wrote: > Move `OldFunc->getArgumentList()` here, delete above. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:289: for (const Argument &OldArg : OldFuncArgs) { On 2015/03/12 18:36:18, JF wrote: > Same, move `NewFunc->args()` here. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:300: // correct the types separately On 2015/03/12 18:36:17, JF wrote: > Where is the type corrected? It's not obvious from the surrounding lines and > below usage of BlindReplace. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:302: for (auto UseIter = Old->use_begin(), E = Old->use_end(); E != UseIter;) { On 2015/03/12 18:36:17, JF wrote: > `for (auto U : Old->uses())` > > Actually, it would be better to use the approach RAUW has of `while > (!use_empty())`. That wouldn't work, I'm not changing the content of the use list, just the uses. It's not exactly RAUW, for that reason https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:334: LastBlock != BIter;) { On 2015/03/12 18:36:19, JF wrote: > for (BasicBlock &BB : NewFunc->getBasicBlockList()) no, because we mutate the list (same goes for the rest of similar comments). https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:339: auto &Ctx = Ret->getContext(); On 2015/03/12 18:36:18, JF wrote: > This should be loop invariant, hoist it. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:342: Ret->eraseFromParent(); On 2015/03/12 18:36:18, JF wrote: > I think this invalidates your Instruction iterator. You should create a worklist > first, and then replace+delete (or create a kill list). That's the reason I increment the iterator first thing - which gets me a valid iterator at all times. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:344: new StoreInst(RetVal, FirstNewArg, NewRet); On 2015/03/12 18:36:16, JF wrote: > Give it a name and copy debug location. > > Also, you can guarantee preferred alignment if you also do the same in the > alloca. Turns out store doesn't accept a name. Otherwise, done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:356: NewCall->setDebugLoc(Orig->getDebugLoc()); On 2015/03/12 18:36:17, JF wrote: > Other metadata? I believe this is it, but I added a TODO, because we may want to centralize this copying of metadata for instructions, and push it upstream. I'll investigate this separately. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:362: Orig->getUnwindDest(), Args); On 2015/03/12 18:36:17, JF wrote: > Copy name. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:370: CallInst *Ret = CallInst::Create(Target, Args); On 2015/03/12 18:36:17, JF wrote: > Name. Done, doing this in CopyCallAttributesAndMetadata https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:373: CopyCallAttributesAndMetadata(Orig, Ret); On 2015/03/12 18:36:18, JF wrote: > I think you're missing NoBuiltin, NoInline, ReturnTwice, NotAccessMemory, > OnlyReadsMemory, NotReturn, NotThrow, CannotDuplicate. as per http://llvm.org/docs/LangRef.html#i-call: "8. The optional function attributes list. Only ‘noreturn‘, ‘nounwind‘, ‘readonly‘ and ‘readnone‘ attributes are valid here." https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:388: FunctionType *NewType = dyn_cast<FunctionType>( On 2015/03/12 18:36:16, JF wrote: > cast<FunctionType> Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:400: AllocaInst *Alloca = new AllocaInst(OldRetType); On 2015/03/12 18:36:18, JF wrote: > Alignment to preferred size. > > Name. > > You can also pass OldCall instead of doing insertBefore below. It's simpler to use takeName than cache the old name and call the AllocaInst constructor with the name and Instruction *InsertBefore signature. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:405: (Instruction *)nullptr); On 2015/03/12 18:36:18, JF wrote: > Alignment. Done. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:408: } On 2015/03/12 18:36:18, JF wrote: > Why not create the new call between the alloca and the load? You'd be > constructing in a more intuitive order, and could use a straight IRBuilder. the call is between the alloca and the load *if* we have the return refactoring, otherwise there's nothing to "in between". Or maybe I don't understand your feedback?
Patchset #5 (id:80001) has been deleted
I'm not insisting on it, but I've found that in general using IRBuilder simplifies the code quite a bit (forces it to be written in the order you'd generate code), and it also happens to have nice peepholes built into it which generate better code. https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:130: return MappedTypes[Ty]; On 2015/03/13 22:04:52, Mircea Trofin wrote: > On 2015/03/12 18:36:18, JF wrote: > > auto Found = MappedTypes.find(Ty); > > if (Found != MappedTypes.end()) > > return *Found; > > or the location alternative, to avoid the double search :) There's no point in having two separate functions now, I'd just drop "Internal". https://codereview.chromium.org/992493002/diff/40001/lib/Transforms/NaCl/Simp... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:408: } On 2015/03/13 22:04:52, Mircea Trofin wrote: > On 2015/03/12 18:36:18, JF wrote: > > Why not create the new call between the alloca and the load? You'd be > > constructing in a more intuitive order, and could use a straight IRBuilder. > > the call is between the alloca and the load *if* we have the return refactoring, > otherwise there's nothing to "in between". Or maybe I don't understand your > feedback? I mean that the order of code in this pass should mimic the order that code can get generated in. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:145: LLVMContext &Ctx = Ty->getContext(); You can have a single getContext in this file in runOnModule, and pass it to each function that needs it. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:154: // the new FT has void for the return type Capitalize and punctuation on comments, here and elsewhere. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:198: if (CandidateMapping == MappedTypes.end()) { I would flip the polarity of this: if (... != end) { ... return ...; } // Rest goes here. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:203: Twine NewName = StructTy->getStructName() + ".simplified"; You can 's/Twine/StringRef/' and drop the Storage. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:216: Type *CurrentlyMapped = CandidateMapping->second; You could cast<StructType>(CandidateMapping->second) here, drop the assert and simplify Changed's get* methods (which themselves contain a cast<StructType>). https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:249: assert(NewStruct); You can just use cast<> instead of dyn_cast<>, and drop the assert. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:306: } Personal style, I'd go with: NewArg->setName(OldArg.getName() + ( OldArg.getType()->isAggregateType() ? ".ptr" : "")); https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:336: } Same persona style here, I'd do: bool isAggregateToPtr = ...; BlindReplace(Old, isAggregateToPtr ? new LoadInst(...) : New); https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:357: Ret = nullptr; No point in setting Ret to nullptr since it's going out of scope. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:479: OldCall = NULL; nullptr, but there's no point in doing this since it's going out of scope. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:539: assert(NewFT); cast<FunctionType> above, drop the assert. https://codereview.chromium.org/992493002/diff/100001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/100001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:78: ; CHECK-NEXT: %direct_def.simplified = type { void (%struct*)*, %struct } Move each check group next to their respective code (so, this on line 8, and the below two_param_func next to two_param_func above.
https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:145: LLVMContext &Ctx = Ty->getContext(); On 2015/03/14 18:42:58, JF wrote: > You can have a single getContext in this file in runOnModule, and pass it to > each function that needs it. Done. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:154: // the new FT has void for the return type On 2015/03/14 18:42:58, JF wrote: > Capitalize and punctuation on comments, here and elsewhere. Done. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:203: Twine NewName = StructTy->getStructName() + ".simplified"; On 2015/03/14 18:42:58, JF wrote: > You can 's/Twine/StringRef/' and drop the Storage. If StructType::create accepted a Twine &, that'd work, but it wants a StringRef https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:249: assert(NewStruct); On 2015/03/14 18:42:58, JF wrote: > You can just use cast<> instead of dyn_cast<>, and drop the assert. Done. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:306: } On 2015/03/14 18:42:58, JF wrote: > Personal style, I'd go with: > NewArg->setName(OldArg.getName() + ( > OldArg.getType()->isAggregateType() ? ".ptr" : "")); Done. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:336: } On 2015/03/14 18:42:59, JF wrote: > Same persona style here, I'd do: > bool isAggregateToPtr = ...; > BlindReplace(Old, isAggregateToPtr ? new LoadInst(...) : New); Done. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:357: Ret = nullptr; On 2015/03/14 18:42:58, JF wrote: > No point in setting Ret to nullptr since it's going out of scope. Maintainability - makes it clear that Ret is invalid after eraseFromParent() https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:479: OldCall = NULL; On 2015/03/14 18:42:58, JF wrote: > nullptr, but there's no point in doing this since it's going out of scope. Just maintainability, clarifies the effect of eraseFromParent().
https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:154: // the new FT has void for the return type On 2015/03/16 18:38:45, Mircea Trofin wrote: > On 2015/03/14 18:42:58, JF wrote: > > Capitalize and punctuation on comments, here and elsewhere. > > Done. Punctuation is still missing in a few places. https://codereview.chromium.org/992493002/diff/100001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:203: Twine NewName = StructTy->getStructName() + ".simplified"; On 2015/03/16 18:38:45, Mircea Trofin wrote: > On 2015/03/14 18:42:58, JF wrote: > > You can 's/Twine/StringRef/' and drop the Storage. > > If StructType::create accepted a Twine &, that'd work, but it wants a StringRef Twine has a StringRef ctor, so it'll work. All you need is: MappedTypes[Ty] = StructType::create(Ctx, StructTy->getStructName() + ".simplified"); https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:146: } I'd leave this as just a call to eraseFromParent. The nullptr setting isn't idiomatic C++ or LLVM-ism :-) https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:163: } Add a comment explaining why exclude struct types here. IIUC code below does the insertion into MappedTypes? Maybe the else clause here should assert that MappedTypes.find(Ty) != MappedTypes.end()? https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:238: if (!StructTy->isLiteral()) { Explain why not literals. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:391: Store->setDebugLoc(Ret->getDebugLoc()); No need for debug loc with IRBuilder. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:453: NewType, Alloca); `auto *` is more idiomatic. https://codereview.chromium.org/992493002/diff/160001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/160001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:61: ; CHECK: define void @returns_struct(%struct* sret %retVal, i32 %an_int, %struct* byval %val.ptr) This should be: ; CHECK-LABEL: @lots_of_call_attrs( Same for other labels.
https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:29: #include <cassert> standard #includes go after LLVM #includes: http://llvm.org/docs/CodingStandards.html#include-style
https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:29: #include <cassert> On 2015/03/16 22:48:33, Derek Schuff wrote: > standard #includes go after LLVM #includes: > http://llvm.org/docs/CodingStandards.html#include-style Done. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:146: } On 2015/03/16 21:53:47, JF wrote: > I'd leave this as just a call to eraseFromParent. The nullptr setting isn't > idiomatic C++ or LLVM-ism :-) Done. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:163: } On 2015/03/16 21:53:47, JF wrote: > Add a comment explaining why exclude struct types here. IIUC code below does the > insertion into MappedTypes? Maybe the else clause here should assert that > MappedTypes.find(Ty) != MappedTypes.end()? Done. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:238: if (!StructTy->isLiteral()) { On 2015/03/16 21:53:47, JF wrote: > Explain why not literals. Done. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:391: Store->setDebugLoc(Ret->getDebugLoc()); On 2015/03/16 21:53:47, JF wrote: > No need for debug loc with IRBuilder. Done. https://codereview.chromium.org/992493002/diff/160001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:453: NewType, Alloca); On 2015/03/16 21:53:47, JF wrote: > `auto *` is more idiomatic. Done.
jvoung@chromium.org changed reviewers: + jvoung@chromium.org
https://codereview.chromium.org/992493002/diff/200001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/200001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:626: // Delete leftover functions - the ones with old signatures. Here is your first(?) driveby review =) I didn't really look over the patch, but... note that debug info (subprogram metadata) will refer to the old functions too, so remember to update those as well. E.g., https://codereview.chromium.org/809483003/diff/20001/lib/Transforms/NaCl/Repl... This was something we didn't notice when the PNaCl transformation passes were first written, and I happened to notice this requirement in the LLVM 3.6 code when the Debug info code started checking for this (and silently dropping debug info if it was missing!).
Patchset #11 (id:220001) has been deleted
lgtm with some nits https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:29: #include <llvm/ADT/SmallString.h> make all the includes use "" instead of <> https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:98: DenseMap<StructType *, StructType *> &Tentatives); you've got a lot of DenseMap<StructType *, StructType *> everywhere; use a typedef for a more readable name? (also any other types for which it makes sense; there are no typedefs at all in this file). https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:437: FunctionType *NewType = cast<FunctionType>( NewType, AllocaInst, and LoadInst could all be auto * https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:577: FunctionType *OldFT = OldFunc->getFunctionType(); these could probably both be auto * since it's obvious what they are https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:582: Function *NewFunc = Function::Create(NewFT, OldFunc->getLinkage()); and here https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:614: auto DISubprogramMap = makeSubprogramMap(M); could be auto &DISubprogramMap to make more clear that there's no copying https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:618: for (Module::iterator Iter = M.begin(), E = M.end(); Iter != E;) { why not range-for here too?
https://codereview.chromium.org/992493002/diff/200001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/200001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:626: // Delete leftover functions - the ones with old signatures. On 2015/03/17 16:25:45, jvoung wrote: > Here is your first(?) driveby review =) > > I didn't really look over the patch, but... note that debug info (subprogram > metadata) will refer to the old functions too, so remember to update those as > well. > > E.g., > https://codereview.chromium.org/809483003/diff/20001/lib/Transforms/NaCl/Repl... > > This was something we didn't notice when the PNaCl transformation passes were > first written, and I happened to notice this requirement in the LLVM 3.6 code > when the Debug info code started checking for this (and silently dropping debug > info if it was missing!). Thanks! https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:98: DenseMap<StructType *, StructType *> &Tentatives); On 2015/03/17 21:21:09, Derek Schuff wrote: > you've got a lot of DenseMap<StructType *, StructType *> everywhere; use a > typedef for a more readable name? (also any other types for which it makes > sense; there are no typedefs at all in this file). Done. https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:437: FunctionType *NewType = cast<FunctionType>( On 2015/03/17 21:21:09, Derek Schuff wrote: > NewType, AllocaInst, and LoadInst could all be auto * Done. https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:582: Function *NewFunc = Function::Create(NewFT, OldFunc->getLinkage()); On 2015/03/17 21:21:09, Derek Schuff wrote: > and here Done. https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:614: auto DISubprogramMap = makeSubprogramMap(M); On 2015/03/17 21:21:09, Derek Schuff wrote: > could be auto &DISubprogramMap to make more clear that there's no copying The API (llvm::makeSubprogramMap) isn't returning a ref though. https://codereview.chromium.org/992493002/diff/240001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:618: for (Module::iterator Iter = M.begin(), E = M.end(); Iter != E;) { On 2015/03/17 21:21:09, Derek Schuff wrote: > why not range-for here too? Mutation
https://codereview.chromium.org/992493002/diff/260001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/260001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:1: ; RUN: opt %s -simplify-struct-reg-signatures -S | FileCheck %s Lacking tests: - ArrayType - VectorType - Vararg function that gets rewritten And nesting of these with other types.
Patchset #13 (id:280001) has been deleted
Additional tests + fix for proper handling of invoke when return is register, by adjusting the normal landing. The exception landing pad block is left as-is, since we don't know what the state of the return would be. https://codereview.chromium.org/992493002/diff/260001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/260001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:1: ; RUN: opt %s -simplify-struct-reg-signatures -S | FileCheck %s On 2015/03/18 20:27:57, JF wrote: > Lacking tests: > - ArrayType > - VectorType > - Vararg function that gets rewritten > And nesting of these with other types. Done.
Sorry for not lgtm, I'm still finding stuff :( https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:443: auto *Alloca = Builder.CreateAlloca(OldRetType); I just realized: the alloca needs to be on the function's entry block, otherwise you're potentially doing alloca in a loop. Look at ExpandVarArgs.cpp which does the same thing. https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:451: if (auto *Invoke = dyn_cast<InvokeInst>(OldCall)){ clang-format https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:453: IRBuilder<> ContBuilder(ContBlock->getFirstInsertionPt()); You could just change the insert point for the existing builder: if (invoke) Builder.setInsertionPoint(ContBlock->first()); auto *Load = Builder.CreateLoad(...); https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:492: Builder.CreateAlloca(OldArgType, nullptr, OldArg->getName() + ".ptr"); Same thing on the alloca. https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:59: %tmp2 = invoke %struct @struct_returning_extern(i32 1, %struct %tmp) to label %Cont unwind label %Cleanup Put the "to label ..." on the next line, it'll be easier to read. https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:87: ; CHECK-NEXT: %exn = landingpad { i8*, i32 } personality i32 (...)* @__gxx_personality_v0 Could you also add a separate test for resume with a struct, and landingpad with one? https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:170: ;CHECK-LABEL declare void @vararg_fp_fct(%vararg_fp_struct.simplified* byval %arg) Here and below you're missing the colons for `CHECK:` so it's not checking :) https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:181: ; CHECK-NEXTcall void (i32, ...)* %fptr(i32 0) The second param disappeared?
Patchset #14 (id:320001) has been deleted
https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:443: auto *Alloca = Builder.CreateAlloca(OldRetType); On 2015/03/19 16:21:25, JF wrote: > I just realized: the alloca needs to be on the function's entry block, otherwise > you're potentially doing alloca in a loop. Look at ExpandVarArgs.cpp which does > the same thing. Done. https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:451: if (auto *Invoke = dyn_cast<InvokeInst>(OldCall)){ On 2015/03/19 16:21:25, JF wrote: > clang-format Done. https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:453: IRBuilder<> ContBuilder(ContBlock->getFirstInsertionPt()); On 2015/03/19 16:21:25, JF wrote: > You could just change the insert point for the existing builder: > > if (invoke) > Builder.setInsertionPoint(ContBlock->first()); > auto *Load = Builder.CreateLoad(...); Done. https://codereview.chromium.org/992493002/diff/300001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:492: Builder.CreateAlloca(OldArgType, nullptr, OldArg->getName() + ".ptr"); On 2015/03/19 16:21:25, JF wrote: > Same thing on the alloca. Done. There was an implementation option to first get all the allocas in the right place, then jump to the "call" insert point and plug in the stores. However, I think it is easier to understand if the code coordinates from the old parameters rather than the codegen being produced. https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-signatures.ll (right): https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:59: %tmp2 = invoke %struct @struct_returning_extern(i32 1, %struct %tmp) to label %Cont unwind label %Cleanup On 2015/03/19 16:21:25, JF wrote: > Put the "to label ..." on the next line, it'll be easier to read. Done. https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:87: ; CHECK-NEXT: %exn = landingpad { i8*, i32 } personality i32 (...)* @__gxx_personality_v0 On 2015/03/19 16:21:25, JF wrote: > Could you also add a separate test for resume with a struct, and landingpad with > one? We decided not to support that scenario, explicitly. Added tests for that. https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:170: ;CHECK-LABEL declare void @vararg_fp_fct(%vararg_fp_struct.simplified* byval %arg) On 2015/03/19 16:21:25, JF wrote: > Here and below you're missing the colons for `CHECK:` so it's not checking :) Done. https://codereview.chromium.org/992493002/diff/300001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-signatures.ll:181: ; CHECK-NEXTcall void (i32, ...)* %fptr(i32 0) On 2015/03/19 16:21:25, JF wrote: > The second param disappeared? Yes, and fixed the bug. Thanks!
https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-pad-crash.ll (right): https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-pad-crash.ll:7: declare %struct @__hypothetical_personality_2(i32) I'll remove this second personality, actually. I could create another test file with it, but it might be overkill.
https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:490: // correctly deal with varargs scenarios. We decided to avoid handling struct passed in vararg because C disallows it. IR doesn't disallow it, so could you make sure you report fatal error on this? https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:510: Builder.SetInsertPoint(SavedInsPoint); I think it would be better to pull this out as a function which creates the alloca, and returns a tuple with {Alloca, InsPoint}, and use the function here and above. https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:673: if (LType != Mapper.getSimpleType(Ctx, LType)) Before report_fatal_error you should: errs() << *Landing << '\n'; Same below. https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:676: true); You don't need llvm:: here, and true is the default so you can drop it. https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-pad-crash.ll (right): https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-pad-crash.ll:2: ; REQUIRES: asserts Do you actually need this? It'll still crash even in a no-assert build because report_fatal_error doesn't get removed. https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-resume-crash.ll (right): https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-resume-crash.ll:2: ; REQUIRES: asserts Same.
https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:490: // correctly deal with varargs scenarios. On 2015/03/20 16:41:32, JF wrote: > We decided to avoid handling struct passed in vararg because C disallows it. IR > doesn't disallow it, so could you make sure you report fatal error on this? Done. https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:510: Builder.SetInsertPoint(SavedInsPoint); On 2015/03/20 16:41:32, JF wrote: > I think it would be better to pull this out as a function which creates the > alloca, and returns a tuple with {Alloca, InsPoint}, and use the function here > and above. Done. https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:673: if (LType != Mapper.getSimpleType(Ctx, LType)) On 2015/03/20 16:41:32, JF wrote: > Before report_fatal_error you should: > errs() << *Landing << '\n'; > > Same below. Done. https://codereview.chromium.org/992493002/diff/340001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:676: true); On 2015/03/20 16:41:32, JF wrote: > You don't need llvm:: here, and true is the default so you can drop it. Done. https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-pad-crash.ll (right): https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-pad-crash.ll:2: ; REQUIRES: asserts On 2015/03/20 16:41:32, JF wrote: > Do you actually need this? It'll still crash even in a no-assert build because > report_fatal_error doesn't get removed. Done. https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... File test/Transforms/NaCl/simplify-struct-reg-resume-crash.ll (right): https://codereview.chromium.org/992493002/diff/340001/test/Transforms/NaCl/si... test/Transforms/NaCl/simplify-struct-reg-resume-crash.ll:2: ; REQUIRES: asserts On 2015/03/20 16:41:32, JF wrote: > Same. Done.
lgtm *✲ﾟ*｡✧٩(･ิᴗ･ิ๑)۶*✲ﾟ*｡✧ after fixing two small nits. https://codereview.chromium.org/992493002/diff/360001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/360001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:424: static AllocaInst *InsertAllocaAtLocation(IRBuilder<> &Builder, Keep the comment that explains why alloca goes at the start of the function. https://codereview.chromium.org/992493002/diff/360001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:689: true); Drop true here and above.
https://codereview.chromium.org/992493002/diff/360001/lib/Transforms/NaCl/Sim... File lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp (right): https://codereview.chromium.org/992493002/diff/360001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:424: static AllocaInst *InsertAllocaAtLocation(IRBuilder<> &Builder, On 2015/03/20 20:47:48, JF wrote: > Keep the comment that explains why alloca goes at the start of the function. Done. https://codereview.chromium.org/992493002/diff/360001/lib/Transforms/NaCl/Sim... lib/Transforms/NaCl/SimplifyStructRegSignatures.cpp:689: true); On 2015/03/20 20:47:48, JF wrote: > Drop true here and above. Done.
Message was sent while issue was closed.
Committed patchset #16 (id:380001) manually as 6fbcecb7791690603fe502d3469cf6b639499e37 (presubmit successful). |
