Issue39 #46
Issue39 #46
Conversation
…he CilkR runtime system.
…lus runtime system.
| @@ -247,7 +247,7 @@ class StructType : public CompositeType { | |||
|
|
|||
| /// Try to lookup a structure type by name, and create one if one does not | |||
| /// exist. | |||
| static StructType *getOrCreate(LLVMContext &Context, StringRef Name); | |||
| static StructType *lookupOrCreate(LLVMContext &Context, StringRef Name); | |||
|
|
|||
There was a problem hiding this comment.
So I was the one who initially added getOrCreate to this file. I'm confident that this method belongs in this file. But after reading the documentation about the other get methods in this file, I didn't feel comfortable continuing to use get in the name of this method. This change might not be strictly necessary, but I think it helps differentiate the semantics of this method from other methods with similar names.
| @@ -25,7 +25,6 @@ | |||
| #include "llvm/Transforms/Utils/ValueMapper.h" | |||
|
|
|||
| namespace llvm { | |||
There was a problem hiding this comment.
Was there a reason these things were moved from a tapir namespace? I'm personally leaning towards keeping it like that.
There was a problem hiding this comment.
After reorganizing the CilkABI code to deal with the first two goals, I observed that Tapir/LLVM would fail to build on old versions of GCC. The problem stems from a bug in how GCC handles namespaces. This GCC bug was not fixed until recently, i.e., GCC version 7. The workaround I found involves removing the Tapir namespace. Given that a lot of people who want to build Tapir still use older versions of GCC, I believe we need to accommodate those older GCC versions.
There was a problem hiding this comment.
I'm also not clear on why the Tapir namespace was added in the first place. It seemed to be pushed to the codebase in a commit that never received review. It also added substantial clutter and redundancy to Tapir-related code and to parts of the codebase that interact with Tapir. Namespaces are used elsewhere in LLVM, but pretty sparingly, from what I see, and in ways that add clarity, not clutter. I don't feel the Tapir namespace accomplished that.
| for (Function &F : M) { | ||
| if (F.getName() == "main") | ||
| WorkList.push_back(&F); | ||
|
|
There was a problem hiding this comment.
I feel like a main-specific pass should be a separate function in TapirTarget
There was a problem hiding this comment.
This change is actually a bug fix to the Tapir lowering pass. Some Tapir targets already contain code that expects to see main among the functions to process. But if main itself did not contain a detach, then this routine in TapirToTarget would prevent those Tapir targets from properly process main, leading to incorrect behavior. We can separate out main-specific processing, but we'll need to fix some specific targets as well. I think that change makes sense to put in another PR.
…essing of 'main' in a separate Tapir-target method.
These commits accomplish a few goals:
Accomplishing these goals involves similar changes to the codebase, which is why these commits address all three.