Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
460053003:
PNaCl: Handle nested structure types in -expand-struct-regs. (Closed)
|
Created:
6 years, 4 months ago by Richard Diamond Modified:
5 years, 12 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. |
DescriptionPNaCl: Handle nested structure types in -expand-struct-regs.
This pass now runs in one or more passes, triggered if splitting any instruction
inserts another struct instruction. ExpandExtractValues now handles n-arty trees.
Also, preserve load/store alignments, if specified.
-expand-struct-regs now exhibits O(mn) complexity, where m is the height of the
type tree (not a graph; broken by pointer) and n is the instruction count.
This pass could actually be split in two: first part handling load/store/phi,
the second handling expandvalue/insertvalue.
This is for Rust, which generates quite a bit of nesting.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3815
R=jfb@chromium.org, mseaborn@chromium.org
TEST= (cd toolchain_build/out/llvm_i686_linux_work && make check)
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=9eecd8225bfd180b9160d838b3049175780bcf09
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fix nits. #Patch Set 3 : Forgot a small change. #
Total comments: 20
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Fix incorrect comment. #
Total comments: 6
Patch Set 7 : #Patch Set 8 : #
Total comments: 8
Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 25 (0 generated)
Adding Mark to the review since he's the original author. He's away for a few days and should be back later this week. Thanks for the patch Richard.
Adding BUG https://code.google.com/p/nativeclient/issues/detail?id=3815 to this issue.
On 2014/09/03 16:45:20, JF wrote: > Adding BUG https://code.google.com/p/nativeclient/issues/detail?id=3815 to this > issue. I planned on adding invoke and array expansion support in subsequent patches.
Is this blocked on something? I'd like to move this forward.
I guess it fell off Mark's review radar, thanks for the ping. Yes, let's move this forward.
On 2014/12/02 17:28:15, JF wrote: > I guess it fell off Mark's review radar, thanks for the ping. Yes, let's move > this forward. Would it be possible to get a timeframe on this?
Here's a first pass since we haven't heard from Mark yet. Note that we've moved to LLVM 3.5 so C++11 would be better. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:68: static void SplitUpPHINode(PHINode *Phi, bool &NeedsAnotherPass) { I'd return a bool here instead, same with the other functions. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:210: Value *ResultField = NULL; nullptr https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:233: if (ResultField != NULL) { if (ResultField) https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:262: assert(ResultField != NULL); assert(ResultField); https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:276: for (BasicBlock::iterator Iter = BB->begin(), E = BB->end(); Iter != E;) { C++11 range-based for here. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:295: I != E; ++I) { Range-based. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:300: I != E; ++I) { Range-based. https://codereview.chromium.org/460053003/diff/1/test/Transforms/NaCl/expand-... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/1/test/Transforms/NaCl/expand-... test/Transforms/NaCl/expand-struct-regs.ll:143: store { { i32, i64 } } %h, { { i32, i64 } }* %a3 Could you also add tests for phi and load? https://codereview.chromium.org/460053003/diff/1/test/Transforms/NaCl/expand-... test/Transforms/NaCl/expand-struct-regs.ll:144: unreachable Why end with unreachable?
https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:68: static void SplitUpPHINode(PHINode *Phi, bool &NeedsAnotherPass) { On 2014/12/06 23:52:13, JF wrote: > I'd return a bool here instead, same with the other functions. Done. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:210: Value *ResultField = NULL; On 2014/12/06 23:52:13, JF wrote: > nullptr Done. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:233: if (ResultField != NULL) { On 2014/12/06 23:52:13, JF wrote: > if (ResultField) Done. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:262: assert(ResultField != NULL); On 2014/12/06 23:52:13, JF wrote: > assert(ResultField); Done. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:276: for (BasicBlock::iterator Iter = BB->begin(), E = BB->end(); Iter != E;) { On 2014/12/06 23:52:13, JF wrote: > C++11 range-based for here. The inner loop can't use a range-based for loop because we need to store the next position before possibly deleting an extractvalue instruction. The outer loop now uses a range-based for loop. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:295: I != E; ++I) { On 2014/12/06 23:52:13, JF wrote: > Range-based. Done. https://codereview.chromium.org/460053003/diff/1/lib/Transforms/NaCl/ExpandSt... lib/Transforms/NaCl/ExpandStructRegs.cpp:300: I != E; ++I) { On 2014/12/06 23:52:13, JF wrote: > Range-based. Done. https://codereview.chromium.org/460053003/diff/1/test/Transforms/NaCl/expand-... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/1/test/Transforms/NaCl/expand-... test/Transforms/NaCl/expand-struct-regs.ll:143: store { { i32, i64 } } %h, { { i32, i64 } }* %a3 On 2014/12/06 23:52:13, JF wrote: > Could you also add tests for phi and load? Done. https://codereview.chromium.org/460053003/diff/1/test/Transforms/NaCl/expand-... test/Transforms/NaCl/expand-struct-regs.ll:144: unreachable On 2014/12/06 23:52:14, JF wrote: > Why end with unreachable? No reason. Fixed.
I assume you're rebased and reran tests? I think it'll be good after the last few changes. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:338: NeedsAnotherPass = SplitUpStore(Store) || NeedsAnotherPass; I think this is cleaner: NeedsAnotherPass |= foo(); https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:364: return (NeedsAnotherPass && runOnFunction(Func)) || Changed; I'm not a fan of the tail recursion, could you just make it a loop, and then the ExpandExtractValues call can be after the loop. You could just return Changed here.
Yes, this has been rebased and I ran the tests. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:338: NeedsAnotherPass = SplitUpStore(Store) || NeedsAnotherPass; On 2014/12/09 19:20:22, JF wrote: > I think this is cleaner: > NeedsAnotherPass |= foo(); Done. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:364: return (NeedsAnotherPass && runOnFunction(Func)) || Changed; On 2014/12/09 19:20:22, JF wrote: > I'm not a fan of the tail recursion, could you just make it a loop, and then the > ExpandExtractValues call can be after the loop. You could just return Changed > here. Done.
[In decreasing order of importance, I'm reviewing for: * Correctness -- If the pass doesn't abort, does it generate correct code? We want to avoid miscompiles because they're hard to debug. * Fail-safety -- e.g. What if assert() is compiled out? * Completeness -- It's not essential for the pass to handle all cases, though it's better if it does or if it's clear what cases are not handled. * Readability * Style] https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:225: size_t I = 0; "I" is too cryptic for an index used across multiple iterations in complex ways. Something like "EVIndex" (and "IVIndex" for "J") would be better. What invariants apply to "I" across the loop? https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:234: ++J, ++I) { This doesn't look right. You increment "I", but "I" never gets reset. This code has to search multiple InsertValueInsts until it finds a matching one. What happens if an InsertValueInst only partially matches? Will "I" have been advanced when we start checking a later InsertValueInst? I find this new version of ExpandExtractValue() hard to understand. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:265: } Can we reach after this "}" with StructVal not having been modified? If so, this would go into an infinite loop. It's not really clear to me whether there's an input that could cause this. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:280: assert(ResultField); Are there inputs for which this assertion would fail? https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:297: Instruction* Inst = Iter++; Nit: LLVM style is " *" https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:301: ToErase.push_back(Inst); ExpandExtractValue() can create InsertValueInsts, but it looks like they won't get added to ToErase, which runs the risk of causing a failure later. I guess this explains why you do "if (!NeedsAnotherPass)" immediately below. This isn't very obvious, though. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:328: // Split up aggregate loads, stores and phi nodes into operations on I think there's a completeness problem here: This no longer splits up loads/stores/phis into operations on scalar types. Since this loop is not iterated, doesn't it do only one level of splitting? Shouldn't this loop be iterated so that the comment is accurate? Or maybe each call to SplitUpStore(), SplitUpLoad() etc. should be iterated? https://codereview.chromium.org/460053003/diff/40001/test/Transforms/NaCl/exp... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/40001/test/Transforms/NaCl/exp... test/Transforms/NaCl/expand-struct-regs.ll:158: define void @nested_structs_phi_load() { I'm not sure what this test case is intended to test. It doesn't use nested structs except for alloca & getelementptr, which I think work without this changeset applied (since the pass doesn't change alloca/getelementptr). Is this perhaps testing a corner case you found during development? If so, that would be worth commenting.
https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:225: size_t I = 0; On 2014/12/09 20:39:46, Mark Seaborn wrote: > "I" is too cryptic for an index used across multiple iterations in complex ways. > Something like "EVIndex" (and "IVIndex" for "J") would be better. Done. > What invariants apply to "I" across the loop? I've added a comment explicating. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:234: ++J, ++I) { On 2014/12/09 20:39:46, Mark Seaborn wrote: > This doesn't look right. You increment "I", but "I" never gets reset. This > code has to search multiple InsertValueInsts until it finds a matching one. > What happens if an InsertValueInst only partially matches? Will "I" have been > advanced when we start checking a later InsertValueInst? Yes, see line 242 ((EVIndex + 1 == EV->getIndices().size())'s 'else' block) > I find this new version of ExpandExtractValue() hard to understand. I apologize; I went ahead and added many comments w/ examples explaining all the cases handled in this section. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:265: } On 2014/12/09 20:39:46, Mark Seaborn wrote: > Can we reach after this "}" with StructVal not having been modified? If so, > this would go into an infinite loop. It's not really clear to me whether > there's an input that could cause this. No, because all such cases would be caught by the branch at line 253 (EVIndex == EV->getIndices().size()) (the inner loop only breaks if it sets ResultValue, StructVal, or EVIndex < EV->getIndices().size(). In the later case, the aforementioned branch would be hit). https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:280: assert(ResultField); On 2014/12/09 20:39:46, Mark Seaborn wrote: > Are there inputs for which this assertion would fail? There shouldn't be, but that doesn't prevent a dev (such as myself!) from making such a mistake in the future. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:297: Instruction* Inst = Iter++; On 2014/12/09 20:39:46, Mark Seaborn wrote: > Nit: LLVM style is " *" Whoops. Done. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:301: ToErase.push_back(Inst); On 2014/12/09 20:39:46, Mark Seaborn wrote: > ExpandExtractValue() can create InsertValueInsts, but it looks like they won't > get added to ToErase, which runs the risk of causing a failure later. > > I guess this explains why you do "if (!NeedsAnotherPass)" immediately below. > This isn't very obvious, though. Actually, extra passes in ExpandExtractValues are impossible because ExpandExtractValue never inserts ExtractValueInsts. I removed the loop. https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... lib/Transforms/NaCl/ExpandStructRegs.cpp:328: // Split up aggregate loads, stores and phi nodes into operations on On 2014/12/09 20:39:46, Mark Seaborn wrote: > I think there's a completeness problem here: This no longer splits up > loads/stores/phis into operations on scalar types. Since this loop is not > iterated, doesn't it do only one level of splitting? > > Shouldn't this loop be iterated so that the comment is accurate? > > Or maybe each call to SplitUpStore(), SplitUpLoad() etc. should be iterated? I was using recursion, however this patchset is old; JF had me switch to a looped impl. https://codereview.chromium.org/460053003/diff/40001/test/Transforms/NaCl/exp... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/40001/test/Transforms/NaCl/exp... test/Transforms/NaCl/expand-struct-regs.ll:158: define void @nested_structs_phi_load() { On 2014/12/09 20:39:46, Mark Seaborn wrote: > I'm not sure what this test case is intended to test. It doesn't use nested > structs except for alloca & getelementptr, which I think work without this > changeset applied (since the pass doesn't change alloca/getelementptr). > > Is this perhaps testing a corner case you found during development? If so, that > would be worth commenting. It's not. JF asked for a test with phi and load instructions, and thus @nested_structs_phi_load was born. Removed.
On 2014/12/10 00:58:06, Richard Diamond wrote: > https://codereview.chromium.org/460053003/diff/40001/lib/Transforms/NaCl/Expa... > lib/Transforms/NaCl/ExpandStructRegs.cpp:301: ToErase.push_back(Inst); > On 2014/12/09 20:39:46, Mark Seaborn wrote: > > ExpandExtractValue() can create InsertValueInsts, but it looks like they won't > > get added to ToErase, which runs the risk of causing a failure later. > > > > I guess this explains why you do "if (!NeedsAnotherPass)" immediately below. > > This isn't very obvious, though. > > Actually, extra passes in ExpandExtractValues are impossible because > ExpandExtractValue never inserts ExtractValueInsts. I removed the loop. s/impossible/unnecessary/ I've push another patchset fixing an incorrect comment.
https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... lib/Transforms/NaCl/ExpandStructRegs.cpp:219: static bool ExpandExtractValue(ExtractValueInst *EV, SmallVectorImpl<Instruction*>& ToErase) { Pass in a pointer to the SmallVector instead. https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... lib/Transforms/NaCl/ExpandStructRegs.cpp:238: for (; EVIndex < EV->getIndices().size() && IVIndex < IV->getIndices().size(); Could you re-run clang-format on the code you've changed? https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... lib/Transforms/NaCl/ExpandStructRegs.cpp:357: assert(ResultField); // Failsafe. Failsafe?
https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... File lib/Transforms/NaCl/ExpandStructRegs.cpp (right): https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... lib/Transforms/NaCl/ExpandStructRegs.cpp:219: static bool ExpandExtractValue(ExtractValueInst *EV, SmallVectorImpl<Instruction*>& ToErase) { On 2014/12/10 22:54:56, JF wrote: > Pass in a pointer to the SmallVector instead. Done. https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... lib/Transforms/NaCl/ExpandStructRegs.cpp:238: for (; EVIndex < EV->getIndices().size() && IVIndex < IV->getIndices().size(); On 2014/12/10 22:54:56, JF wrote: > Could you re-run clang-format on the code you've changed? Done. https://codereview.chromium.org/460053003/diff/100001/lib/Transforms/NaCl/Exp... lib/Transforms/NaCl/ExpandStructRegs.cpp:357: assert(ResultField); // Failsafe. On 2014/12/10 22:54:56, JF wrote: > Failsafe? Nevermind, I've removed it.
Shameless self-bump.
Mark: is this ready to land? I'd like to check it in this week, so it has time to be in before the branch in early January.
I think this is close to ready to go. One thing I'd add are tests for phi/select/store/load (one function each in the .ll file) which exercise the NeedsAnotherPass case. I'm assuming the rest looks good to Mark, since we haven't heard back. I'll check this in if you add the tests.
On 2014/12/24 14:28:38, JF wrote: > I think this is close to ready to go. One thing I'd add are tests for > phi/select/store/load (one function each in the .ll file) which exercise the > NeedsAnotherPass case. > > I'm assuming the rest looks good to Mark, since we haven't heard back. I'll > check this in if you add the tests. Done.
Sorry, a few more comments :-s https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:167: ; CHECK-NEXT: %b.field.field = load i64* %b.field.index, align 1 Could you have a test for alignment preservation too, say with { { i8, i8, i64, i32 }} to make sure that each field is aligned as expected (I think this should be 8, 1, (padding), 8, 8). Same on store. PNaCl will later drop integer alignment (and preserve floating-point / vector) but we want these passes to be upstreamable, so they need to be general enough. https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:187: ; CHECK-NEXT: %a.index.index = select i1 undef, i64 undef, i64 undef Could you also test with a selected value of { { i64, i64 } } and then store the value? I want to make sure that both fields get stored. Could you also use globals of type { { i64 } } here instead of undef? Just to make sure parameter order is preserved into the select. https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:199: ; CHECK: %a.index.index = phi i64 [ undef, %entry ] Same as select, please add a phi with { { i64, i64 } }. Could you also have two incoming basic blocks, to ensure order is preserved?
https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:167: ; CHECK-NEXT: %b.field.field = load i64* %b.field.index, align 1 On 2014/12/25 17:09:36, JF wrote: > Could you have a test for alignment preservation too, say with { { i8, i8, i64, > i32 }} to make sure that each field is aligned as expected (I think this should > be 8, 1, (padding), 8, 8). > > Same on store. > > PNaCl will later drop integer alignment (and preserve floating-point / vector) > but we want these passes to be upstreamable, so they need to be general enough. This pass didn't preserve alignment before this PR and I think that such work should be done in another PR. https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:187: ; CHECK-NEXT: %a.index.index = select i1 undef, i64 undef, i64 undef On 2014/12/25 17:09:36, JF wrote: > Could you also test with a selected value of { { i64, i64 } } and then store the > value? I want to make sure that both fields get stored. > > Could you also use globals of type { { i64 } } here instead of undef? Just to > make sure parameter order is preserved into the select. I used a local load from null (lol), so there are unique values for both value operands. https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:199: ; CHECK: %a.index.index = phi i64 [ undef, %entry ] On 2014/12/25 17:09:36, JF wrote: > Same as select, please add a phi with { { i64, i64 } }. Could you also have two > incoming basic blocks, to ensure order is preserved? Done.
lgtm after adding the TODO. https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:167: ; CHECK-NEXT: %b.field.field = load i64* %b.field.index, align 1 On 2014/12/26 01:57:45, Richard Diamond wrote: > On 2014/12/25 17:09:36, JF wrote: > > Could you have a test for alignment preservation too, say with { { i8, i8, > i64, > > i32 }} to make sure that each field is aligned as expected (I think this > should > > be 8, 1, (padding), 8, 8). > > > > Same on store. > > > > PNaCl will later drop integer alignment (and preserve floating-point / vector) > > but we want these passes to be upstreamable, so they need to be general > enough. > > This pass didn't preserve alignment before this PR and I think that such work > should be done in another PR. That's acceptable, though please note that open source often is about improving more than what you came in to improve. In this case it'll require using MathExtras.h:MinAlign and should be about 2 lines of code for load, same on store. I don't mind another patch, but I would like at least a TODO.
https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... File test/Transforms/NaCl/expand-struct-regs.ll (right): https://codereview.chromium.org/460053003/diff/140001/test/Transforms/NaCl/ex... test/Transforms/NaCl/expand-struct-regs.ll:167: ; CHECK-NEXT: %b.field.field = load i64* %b.field.index, align 1 On 2014/12/26 18:14:41, JF wrote: > On 2014/12/26 01:57:45, Richard Diamond wrote: > > On 2014/12/25 17:09:36, JF wrote: > > > Could you have a test for alignment preservation too, say with { { i8, i8, > > i64, > > > i32 }} to make sure that each field is aligned as expected (I think this > > should > > > be 8, 1, (padding), 8, 8). > > > > > > Same on store. > > > > > > PNaCl will later drop integer alignment (and preserve floating-point / > vector) > > > but we want these passes to be upstreamable, so they need to be general > > enough. > > > > This pass didn't preserve alignment before this PR and I think that such work > > should be done in another PR. > > That's acceptable, though please note that open source often is about improving > more than what you came in to improve. In this case it'll require using > MathExtras.h:MinAlign and should be about 2 lines of code for load, same on > store. I don't mind another patch, but I would like at least a TODO. Ah, I was unaware of MathExtras existence. That simplifies things. I've added the alignment preservation.
Message was sent while issue was closed.
Committed patchset #10 (id:180001) manually as 9eecd8225bfd180b9160d838b3049175780bcf09 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
