[llvm-commits] [llvm] r132676 - in /llvm/trunk: lib/CodeGen/SelectionDAG/FastISel.cpp test/CodeGen/X86/fast-isel-agg-constant.ll

Jim Grosbach grosbach at apple.com
Thu Jun 9 13:24:38 PDT 2011


On Jun 9, 2011, at 1:18 PM, Roman Divacky wrote:

> On Thu, Jun 09, 2011 at 12:52:24PM -0700, Jim Grosbach wrote:
>> 
>> On Jun 9, 2011, at 12:48 PM, Roman Divacky wrote:
>> 
>>> On Thu, Jun 09, 2011 at 11:22:41AM -0700, Jim Grosbach wrote:
>>>> 
>>>> On Jun 9, 2011, at 1:08 AM, Roman Divacky wrote:
>>>> 
>>>>> On Wed, Jun 08, 2011 at 11:08:36PM -0700, Eric Christopher wrote:
>>>>>> 
>>>>>> On Jun 6, 2011, at 8:38 AM, Roman Divacky wrote:
>>>>>> 
>>>>>>> OK to commit? Or is there a better way to distinguish those two
>>>>>>> asm dialects?
>>>>>> 
>>>>>> Maybe check the Subtarget instead?
>>>>> 
>>>>> Where do I get it in MCExpr?
>>>> 
>>>> If at all possible, we want to avoid the Subtarget in MC layer operations. There are a few places it's done, but it's a layering violation that we're trying very hard to avoid. Long term, we want to refactor the target machine to allow us to decouple it from MC.
>>>> 
>>>> For purposes of something like this, you would theoretically just use the target triple. That is, if the triple specifies the target is Darwin (or not), behave accordingly. I don't think the triple is explicitly threaded through MC, though, so other than conceptually, I don't think that helps you much.
>>> 
>>> The thing is that in MCExpr there's absolutely nothing related to triples etc.
>>> It's just a pure container for an expression. Nothing from the outside world
>>> is there :)
>>> 
>>> Thats why I introduced the two new variant kinds, to distinguish the two
>>> dialects.
>> 
>> Yeah. It's a bit of an ugly solution, but it's pretty much necessary for now. IIRC, there are precedent examples of the same thing in there. We really need a target-hook that has access to the triple level information for this sort of thing, but there's no such beast right now.
> 
> So is the patch OK to commit?

Yes. Sorry, should have said that explicitly before.

As a minor detail, the comments here should be updated to reflect the different output. If I read things right:
+    VK_PPC_DARWIN_HA16,  // ha16(symbol)
+    VK_PPC_DARWIN_LO16,  // lo16(symbol)
+    VK_PPC_GAS_HA16,     // ha16(symbol)		<--  Should be "ha(symbol)"
+    VK_PPC_GAS_LO16      // lo16(symbol) 		<--  Should be "l(symbol)"

-Jim



More information about the llvm-commits mailing list