Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler crash in ponyc 0.28.0 with LLVM 7+ for certain projects #3140

Closed
EpicEric opened this issue Apr 29, 2019 · 61 comments
Closed

Compiler crash in ponyc 0.28.0 with LLVM 7+ for certain projects #3140

EpicEric opened this issue Apr 29, 2019 · 61 comments

Comments

@EpicEric
Copy link
Contributor

EpicEric commented Apr 29, 2019

Originally reported while working on #3138.

A few Pony projects I tested crash ponyc 0.28.0 on the Writing ./foo.o step when running in Docker. For example, pony-kafka or one of my personal projects. Some projects (for example, pony-stable) compile just fine.

For example, with pony-kafka I get the following logs and the error code 139:

$ docker run -v /tmp/pony-kafka:/src/main ponylang/ponyc:0.28.0 ponyc examples/simple  
Building builtin -> /usr/local/lib/pony/0.28.0/packages/builtin
Building examples/simple -> /src/main/examples/simple
Building net -> /usr/local/lib/pony/0.28.0/packages/net
Building collections -> /usr/local/lib/pony/0.28.0/packages/collections
Building ponytest -> /usr/local/lib/pony/0.28.0/packages/ponytest
Building time -> /usr/local/lib/pony/0.28.0/packages/time
Building random -> /usr/local/lib/pony/0.28.0/packages/random
Building ../../pony-kafka/customlogger -> /src/main/pony-kafka/customlogger
Building ../../pony-kafka -> /src/main/pony-kafka
Building custombuffered -> /src/main/pony-kafka/custombuffered
Building codecs -> /src/main/pony-kafka/custombuffered/codecs
Building ../../utils/bool_converter -> /src/main/pony-kafka/utils/bool_converter
Building itertools -> /usr/local/lib/pony/0.28.0/packages/itertools
Building customnet -> /src/main/pony-kafka/customnet
Building fsm -> /src/main/pony-kafka/fsm
Building options -> /usr/local/lib/pony/0.28.0/packages/options
Building compression -> /src/main/pony-kafka/compression
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
 Descriptors
Optimising
Writing ./simple.o
Stack dump:
0.	Running pass 'Function Pass Manager' on module 'simple'.
1.	Running pass 'Greedy Register Allocator' on function '@3359'

This doesn't happen for the 0.27.0 Docker tag, when using the distribution release of Pony outside of the container, or when using ponyc --debug. For these cases, it will compile (or just fail normally with error code 255 when trying to link the required libraries).

I have yet to test the distribution release of Pony inside the container.

@EpicEric
Copy link
Contributor Author

Removing the make parameters here does not solve the issue.

@mfelsche
Copy link
Contributor

ponyc in the Dockerfile is built using the release llvm from releases.llvm.org.
We should switch to using vendored llvm there too.

@mfelsche
Copy link
Contributor

This might be related to #3126 ?

@EpicEric
Copy link
Contributor Author

Probably not. They crash on different compilation steps (Writing ./foo.o instead of Optimising), and on different conditions (i.e. both LLVM 7.0.1 in the Ubuntu image and LLVM 5 in #3138 trigger this issue; the other issue only happens on LLVM 3.9.1).

@SeanTAllen
Copy link
Member

This is probably a LLVM 7.0.1 bug?

@jemc
Copy link
Member

jemc commented May 2, 2019

To get the rest of the stack dump info about this, we'd need to reproduce with LLVM compiled in debug mode instead of release mode.

@SeanTAllen
Copy link
Member

@EpicEric Is your project avaiable so I could try this with vendored LLVM 7.0.1 to see if I have the problem?

@EpicEric
Copy link
Contributor Author

EpicEric commented May 2, 2019

Here's my repository: https://github.com/EpicEric/pcs3866-languages-and-compilers

Using ponyc for either building or running on Docker will cause the crash if it doesn't have the --debug flag.

@EpicEric
Copy link
Contributor Author

This issue happens on the new Alpine image ponylang/ponyc:alpine as well, under the same conditions (i.e. in either docker buildor docker run, but not when ponyc has the --debug flag). But the stack dump isn't printed:

$ docker build -t epiceric/basicc .                                                  
Sending build context to Docker daemon  236.5kB
Step 1/4 : FROM ponylang/ponyc:alpine
 ---> 6f50ce3a71c4
Step 2/4 : COPY basicc basicc
 ---> Using cache
 ---> aa409300d747
Step 3/4 : RUN ponyc -o /bin basicc
 ---> Running in af3b55f3a92a
Building builtin -> /usr/local/lib/pony/0.28.0/packages/builtin
Building basicc -> /src/main/basicc
Building collections -> /usr/local/lib/pony/0.28.0/packages/collections
Building ponytest -> /usr/local/lib/pony/0.28.0/packages/ponytest
Building time -> /usr/local/lib/pony/0.28.0/packages/time
Building random -> /usr/local/lib/pony/0.28.0/packages/random
Building cli -> /usr/local/lib/pony/0.28.0/packages/cli
Building collections/persistent -> /usr/local/lib/pony/0.28.0/packages/collections/persistent
Building buffered -> /usr/local/lib/pony/0.28.0/packages/buffered
Building files -> /usr/local/lib/pony/0.28.0/packages/files
Building term -> /usr/local/lib/pony/0.28.0/packages/term
Building promises -> /usr/local/lib/pony/0.28.0/packages/promises
Building strings -> /usr/local/lib/pony/0.28.0/packages/strings
Building signals -> /usr/local/lib/pony/0.28.0/packages/signals
Building capsicum -> /usr/local/lib/pony/0.28.0/packages/capsicum
Building format -> /usr/local/lib/pony/0.28.0/packages/format
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
 Descriptors
Optimising
Writing /bin/basicc.o
The command '/bin/sh -c ponyc -o /bin basicc' returned a non-zero code: 139

@EpicEric
Copy link
Contributor Author

EpicEric commented Jun 3, 2019

This is what git bisect gave me:

68a4984a91d653c4e53316b419143715a206cf15 is the first bad commit
commit 68a4984a91d653c4e53316b419143715a206cf15
Author: Sean T Allen <sean@monkeysnatchbanana.com>
Date:   Sat Mar 9 07:40:46 2019 -0500

    Upgrade Docker image to use LLVM 7.0.1 (#3079)
    
    Part of our general move off of LLVM 3.9.1.

:100644 100644 fd8051e3685fb56cfc8e57785ebf127a7b526fc7 587872f7cb7fb4524c55c306c5da74dd54df807f M	Dockerfile

Which makes sense, since the Dockerfile was updated in this commit.

Here is my git bisect log:

git bisect start
# good: [506acdf720feb92bf20ff1599795641edbfdbbbc] Prep for 0.27.0 release
git bisect good 506acdf720feb92bf20ff1599795641edbfdbbbc
# bad: [4f0fc19f956ef54a4cd146863c7211a6c1d298db] Prep for 0.28.0 release
git bisect bad 4f0fc19f956ef54a4cd146863c7211a6c1d298db
# good: [89b046b93b59fdb5c57b6ce4db04d0e496f62c18] Fix centos-7-3.9.1 release building
git bisect good 89b046b93b59fdb5c57b6ce4db04d0e496f62c18
# bad: [6a85945f2ffab0b1809259f276295831f6591c22] Update CHANGELOG for PR #3086 [skip ci]
git bisect bad 6a85945f2ffab0b1809259f276295831f6591c22
# bad: [c140ebf9a983ba089e5e1a2434446a7e3ac0e9bc] Update CHANGELOG for PR #3079 [skip ci]
git bisect bad c140ebf9a983ba089e5e1a2434446a7e3ac0e9bc
# good: [34c2a28c041e3001801fdabb0812dee056675024] Add missing CHANGELOG note for Jessie deprecation
git bisect good 34c2a28c041e3001801fdabb0812dee056675024
# good: [08bd39eecca8540280a623d98d11b2e0be991d25] Run macOS CI on Cirrus (#3080)
git bisect good 08bd39eecca8540280a623d98d11b2e0be991d25
# bad: [68a4984a91d653c4e53316b419143715a206cf15] Upgrade Docker image to use LLVM 7.0.1 (#3079)
git bisect bad 68a4984a91d653c4e53316b419143715a206cf15
# first bad commit: [68a4984a91d653c4e53316b419143715a206cf15] Upgrade Docker image to use LLVM 7.0.1 (#3079)

@SeanTAllen
Copy link
Member

I get this crash using the vendored LLVM 7.0.1 so this isn't the ABI issue that 7.1.0 addresses. This is definitely a "real bug".

@SeanTAllen
Copy link
Member

Release version stack trace from MacOS where it also happens:

(lldb) bt
* thread #1: tid = 0x16e274, 0x0000000100d21e01 ponyc`llvm::SplitEditor::transferValues() + 603, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100d21e01 ponyc`llvm::SplitEditor::transferValues() + 603
    frame #1: 0x0000000100d2348a ponyc`llvm::SplitEditor::finish(llvm::SmallVectorImpl<unsigned int>*) + 240
    frame #2: 0x0000000100ce26a4 ponyc`(anonymous namespace)::RAGreedy::selectOrSplitImpl(llvm::LiveInterval&, llvm::SmallVectorImpl<unsigned int>&, llvm::SmallSet<unsigned int, 16u, std::__1::less<unsigned int> >&, unsigned int) + 5576
    frame #3: 0x0000000100cdf72c ponyc`(anonymous namespace)::RAGreedy::selectOrSplit(llvm::LiveInterval&, llvm::SmallVectorImpl<unsigned int>&) + 130
    frame #4: 0x0000000100cda381 ponyc`llvm::RegAllocBase::allocatePhysRegs() + 125
    frame #5: 0x0000000100cdf287 ponyc`(anonymous namespace)::RAGreedy::runOnMachineFunction(llvm::MachineFunction&) + 1825
    frame #6: 0x0000000100c5ffbc ponyc`llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 114
    frame #7: 0x00000001013a2e6a ponyc`llvm::FPPassManager::runOnFunction(llvm::Function&) + 358
    frame #8: 0x00000001013a2ff7 ponyc`llvm::FPPassManager::runOnModule(llvm::Module&) + 49
    frame #9: 0x00000001013a32e6 ponyc`llvm::legacy::PassManagerImpl::run(llvm::Module&) + 572
    frame #10: 0x0000000100d6133f ponyc`LLVMTargetMachineEmit(LLVMOpaqueTargetMachine*, LLVMOpaqueModule*, llvm::raw_pwrite_stream&, LLVMCodeGenFileType, char**) + 367
    frame #11: 0x0000000100d61193 ponyc`LLVMTargetMachineEmitToFile + 183
    frame #12: 0x0000000100051ccf ponyc`genobj + 463
    frame #13: 0x0000000100049165 ponyc`genexe + 597
    frame #14: 0x000000010003e29b ponyc`codegen + 219
    frame #15: 0x0000000100082a90 ponyc`generate_passes + 16
    frame #16: 0x0000000100002ad3 ponyc`main + 611
    frame #17: 0x00007fff9100f5ad libdyld.dylib`start + 1
    frame #18: 0x00007fff9100f5ad libdyld.dylib`start + 1

@EpicEric EpicEric changed the title Compiler crash in ponyc 0.28.0 Docker for certain projects Compiler crash in ponyc 0.28.0 with LLVM 7+ for certain projects Jul 10, 2019
@SeanTAllen
Copy link
Member

A bit more info is available if we don't strip debug info:

Writing ./basicc.o
Stack dump:
0.	Running pass 'Function Pass Manager' on module 'basicc'.
1.	Running pass 'Greedy Register Allocator' on function '@collections_HashMap_String_val_t3_U32_val_u4_SyntaxExpressionNumber_ref_SyntaxExpressionVariable_ref_SyntaxExpressionUnary_ref_SyntaxExpressionBinary_ref_u4_SyntaxExpressionNumber_ref_SyntaxExpressionVariable_ref_SyntaxExpressionUnary_ref_SyntaxExpressionBinary_ref_collections_HashEq_String_val_val_ref_update_o3Iooo'

@SeanTAllen
Copy link
Member

SeanTAllen commented Jul 12, 2019

I don't have any previous experience diagnosing LLVM optimization bugs. I do know that line 1400 in genopt.cc: pmb.populateLTOPassManager(lpm); if turned off, we don't have the crash so, definitely related to the LTO pass in some fashion which matches up with the observed crash.

Interestingly, the only thing I can see being added to lpm is from:

  lpm.add(createTargetTransformInfoWrapperPass(
    machine->getTargetIRAnalysis()));

but turning that off doesn't prevent the crash. If anyone can elucidate more on what is going on here, I'd appreciate it.

@SeanTAllen
Copy link
Member

Turning on the "VerifyOutput" option gives us an interesting result for this case:

Optimising
Instruction does not dominate all uses!
  %180 = tail call i8* @pony_alloc_small(i8* %0, i32 0), !dbg !10382
  %178 = getelementptr inbounds i8, i8* %180, i64 8, !dbg !10382
LLVM fatal error: Broken function found, compilation aborted!

@jemc
Copy link
Member

jemc commented Jul 16, 2019

FYI, "does not dominate all uses" basically translates to "this variable gets used in some branch where it has not actually been assigned".
Here's a talk in which the speaker explains how LLVM Dominator Trees work: https://www.youtube.com/watch?v=bNV18Wy-J0U

@jemc
Copy link
Member

jemc commented Jul 16, 2019

So the error message is saying that we generated code to allocate a thing, and somewhere else we generated code to use that allocated pointer, but that was in an execution branch where it was not actually ever assigned.

@jemc
Copy link
Member

jemc commented Jul 16, 2019

@SeanTAllen - So there's basically two possibilities:

  • 1️⃣ Our custom optimization passes are mishandling some edge case and causing the LLVM IR to be transformed in an invalid way (this is my bet right now). If this is the case, the issue should become fixed if you comment-out one or more of our custom optimization passes on these lines:
    pmb.addExtension(PassManagerBuilder::EP_Peephole,
    addMergeReallocPass);
    pmb.addExtension(PassManagerBuilder::EP_Peephole,
    addHeapToStackPass);
    pmb.addExtension(PassManagerBuilder::EP_ScalarOptimizerLate,
    addDispatchPonyCtxPass);
    pmb.addExtension(PassManagerBuilder::EP_ScalarOptimizerLate,
    addMergeMessageSendPass);
  • 2️⃣ We are generating invalid code in the codegen pass. If this is the case, then if you turn on VerifyOutput but turn off all optimizations (basically, ponyc running in --debug mode), you'd still see the same VerifyOutput error you pasted above.

@SeanTAllen
Copy link
Member

@jemc I do not get the VerifyOutput error when running ponyc --debug. For now I am assuming that idea number 2 is not the problem.

@SeanTAllen
Copy link
Member

@jemc turning off all our pony_specific passes does not fix the issue either.

@SeanTAllen
Copy link
Member

With "VerifyOutput" on.
With all Pony specific optimizations off.
And with the following turned off:

  if(!c->opt->library)
   lpm.run(*m);

that is around line 1428 of genopt.cc then we do not get an error of any sort and successfully compile the program.

@SeanTAllen
Copy link
Member

At pmb.OptLevel = 1; there's no issue. Once we go to 2 or higher, there's an issue

@SeanTAllen
Copy link
Member

SeanTAllen commented Jul 18, 2019

Not populating the module pass manager ALSO fixes the issue.

pmb.populateModulePassManager(mpm);

Perhaps an interaction between module pass and link time pass?

Turning either one of them (doesn't matter which) off, "solves" the issue.

@SeanTAllen
Copy link
Member

Running the module pass after the link pass also "solves" the issue:

  if(!c->opt->library)
    lpm.run(*m);

  mpm.run(*m);

Note, that is the ONLY change needed to genopt.cc to get the example code compiling.

@SeanTAllen
Copy link
Member

SeanTAllen commented Jul 18, 2019

In the LLVM 7 MPM code that we are executing there is:

MPM.add(createInstSimplifyLegacyPass());

If that isn't run then with everything else as is, we are fine and there's no crash.
Given that reordering mpm and lpm also fixes the problem then it would see that InstSimplifyLegacyPass interacts poorly with something that is happening in the LLVM lpm code that we end up executing.

It's getting late here so I'm putting this down for now. I'll pick this up later and try to figure out what is being run in LPM that is interacting poorly with the above MPM pass.

@SeanTAllen
Copy link
Member

Pushing conversation on this to next week.

@mshockwave
Copy link
Contributor

@SeanTAllen pinging this issue. Is there any plan on discussing this issue during sync up meeting?

@SeanTAllen
Copy link
Member

@mshockwave we decided a couple of weeks back that in general, we are:

  • continuing to move towards using a vendored LLVM for everything, but particularly releases. until we are there the rest of this doesn't matter

once we are doing releases using a vendored LLVM, any patches like this will be applied at build time.

We need to create an issue for working on applying patches as part of the make process.

We are generally hoping that LLVM 9 support is finished soon and hopefully, this particular issue doesn't exist anymore. That said, @kulibali is working on switching us over to using cmake for all builds so any work on patching should probably be done after that (as it would perhaps need to be repeated), best to discuss that with @kulibali.

@kulibali has currently paused the cmake work because I've been doing a lot of work around messing with CI and build process as part of the move to vendored.

@mshockwave would you be interested in lending a hand in either of these ways:

  • Helping with the LLVM 9 work
  • Figuring out with @kulibali the best path forward on the "patch vendored LLVM" work as it relates to make and cmake

@lenary
Copy link

lenary commented Oct 16, 2019

@mshockwave you should definitely ping that patch on review, and maybe point out that it breaks Pony (which may make people wake up).

That patch has not landed in LLVM 9.0, so you're still going to have to manually apply this patch after the upgrade to 9.0.

@mshockwave
Copy link
Contributor

@mshockwave we decided a couple of weeks back that in general, we are:

continuing to move towards using a vendored LLVM for everything, but particularly releases. until we are there the rest of this doesn't matter

once we are doing releases using a vendored LLVM, any patches like this will be applied at build time.

We need to create an issue for working on applying patches as part of the make process.

Just want to double check: "vendored" here means using the upstream LLVM source code tree, right?

I'm generally happy to see all these changes happen. Both the cmake part and the adoption of vendored LLVM + patching.

@mshockwave would you be interested in lending a hand in either of these ways:

* Helping with the LLVM 9 work

Yes, I'm happy to help that.

* Figuring out with @kulibali the best path forward on the "patch vendored LLVM" work as it relates to make and cmake

Sure, I believe it would be an interesting topic

@mshockwave
Copy link
Contributor

@lenary Yes I will ping on the Phabricator and connecting it with ponyc. At the same time, I'm contacting LLVM Weekly to see if they can put a heads-up regarding that review in the issue next week.

@lenary
Copy link

lenary commented Oct 16, 2019

@mshockwave that sounds like a sensible idea regarding llvm weekly.

I think "vendoring" means pony ships with their own pony-specific copy of LLVM, like rust currently does, rather than relying on the version of an LLVM somewhere else in the system.

@SeanTAllen
Copy link
Member

Vendoring means using the LLVM that we have pulled in as a git-submodule.

See: https://github.com/ponylang/ponyc/tree/master/lib/llvm

@chalcolith
Copy link
Member

Any help with LLVM 9 (#3320) would be welcome, as I don't have a lot of time to work on it.

@mshockwave
Copy link
Contributor

FYI: Good news my patch regarding MemCpyOpt fix just got accepted. It will be in the next LLVM release.

@SeanTAllen
Copy link
Member

Awesome.

@SeanTAllen
Copy link
Member

@mshockwave would you be interested in helping to get us up to date with LLVM support?

@mshockwave
Copy link
Contributor

@SeanTAllen yes I am interested. I will cooperate with @kulibali on issue #3320

@SeanTAllen
Copy link
Member

@mshockwave you are awesome. thanks.

llvm-git-migration pushed a commit to llvm/llvm-project that referenced this issue Oct 21, 2019
…e Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060

llvm-svn: 375403
dtzWill pushed a commit to llvm-mirror/llvm that referenced this issue Oct 21, 2019
…e Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@375403 91177308-0d34-0410-b5e6-96231b3b80d8
earl pushed a commit to earl/llvm-mirror that referenced this issue Oct 24, 2019
…e Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@375403 91177308-0d34-0410-b5e6-96231b3b80d8
@lenary
Copy link

lenary commented Nov 12, 2019

I have added this patch to the list of changes to be backported to LLVM 9.0.1: PR43974.

tstellar pushed a commit to llvm/llvm-project that referenced this issue Nov 16, 2019
------------------------------------------------------------------------
r375403 | lenary | 2019-10-21 03:00:34 -0700 (Mon, 21 Oct 2019) | 30 lines

[MemCpyOpt] Fixing Incorrect Code Motion while Handling Aggregate Type Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060
------------------------------------------------------------------------
@lenary
Copy link

lenary commented Nov 16, 2019

Update: the fix has been merged into the 9.0.1 branch. I'm still not sure the timeline of the 9.0.1 release.

@SeanTAllen
Copy link
Member

Awesome. Thanks for the update @lenary.

arichardson pushed a commit to arichardson/llvm-project that referenced this issue Nov 16, 2019
…e Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060

llvm-svn: 375403
@lenary
Copy link

lenary commented Nov 17, 2019

@SeanTAllen
Copy link
Member

@EpicEric this is fixed for me on master when using a ponyc compiler built using the vendored LLVM. can you confirm?

  • make -f Makefile-lib-llvm default_pic=true -j4

is how I am building said ponyc compiler.

@EpicEric
Copy link
Contributor Author

Confirming that current master doesn't crash with vendored LLVM.

ajohnson-uoregon pushed a commit to ajohnson-uoregon/clang-rewrite-only that referenced this issue Jul 17, 2022
------------------------------------------------------------------------
r375403 | lenary | 2019-10-21 03:00:34 -0700 (Mon, 21 Oct 2019) | 30 lines

[MemCpyOpt] Fixing Incorrect Code Motion while Handling Aggregate Type Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060
------------------------------------------------------------------------
dutiona pushed a commit to dutiona/llvm-project that referenced this issue Dec 21, 2023
------------------------------------------------------------------------
r375403 | lenary | 2019-10-21 03:00:34 -0700 (Mon, 21 Oct 2019) | 30 lines

[MemCpyOpt] Fixing Incorrect Code Motion while Handling Aggregate Type Values

Summary:
When MemCpyOpt is handling aggregate type values, if an instruction (let's call it P) between the targeting load (L) and store (S) clobbers the source pointer of L, it will try to hoist S before P. This process will also hoist S's data dependency instructions.

However, the current implementation has a bug that if one of S's dependency instructions is //also// a user of P, MemCpyOpt will not prevent it from being hoisted above P and cause a use-before-define error. For example, in the newly added test file (i.e. `aggregate-type-crash.ll`), it will try to hoist both `store %my_struct %1, %my_struct* %3` and its dependent, `%3 = bitcast i8* %2 to %my_struct*`, above `%2 = call i8* @my_malloc(%my_struct* %0)`. Creating the following BB:
```
entry:
  %1 = bitcast i8* %4 to %my_struct*
  %2 = bitcast %my_struct* %1 to i8*
  %3 = bitcast %my_struct* %0 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %2, i8* align 4 %3, i64 8, i1 false)
  %4 = call i8* @my_malloc(%my_struct* %0)
  ret void
```
Where there is a use-before-define error between `%1` and `%4`.

Update: The compiler for the Pony Programming Language [also encounter the same bug](ponylang/ponyc#3140)

Patch by Min-Yih Hsu (myhsu)

Reviewers: eugenis, pcc, dblaikie, dneilson, t.p.northover, lattner

Reviewed By: eugenis

Subscribers: lenary, hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66060
------------------------------------------------------------------------
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants