On Sat, 23 Jun 2012 22:28:55 +0100 Tim Northover <t.p.northover at gmail.com> wrote:> On Sat, Jun 23, 2012 at 04:10:51PM -0500, Hal Finkel wrote: > > Working on a target I added this pattern: > > > > def : Pat<(v4i64 (load xoaddr:$src)), > > (QVFCTIDb (QVLFDXb xoaddr:$src))>; > > > > I'd like to fix this so that it works correctly: the TokenFactor > > inputs should be attached to all inner-most instructions. I'm > > guessing this is somewhere in SelectionDAGISel.cpp, but if someone > > has a more-specific idea, I'd appreciate hearing it. > > I believe it's a current TableGen limitation. When generating its DAG > tables for this kind of thing, TableGen gives output instructions that > should take chain a special flag: Opfl_Chain. > > Unfortunately the way it decides which instructions are worthy of this > flag is rather naive: > + If an instruction has a built-in pattern (in the Instruciton > record), it checks whether that makes use of a chain. > + Otherwise, the outer instruction of the Pat gets a chain. > > So if your QVLFDXb instruction doesn't set the Pattern(s?) field, > TableGen won't know it needs the chain.Tim, Correct, the instruction has no pattern of its own.> > There's a big comment just above the test about how the current > situation is rather broken. I'm currently inclined to add a check for > mayLoad and mayStore at that point in TableGeni (see patch), but was > waiting until I could give tests and justification on the list before > submitting it.Thanks for sending this! As far as a long-term solution, would it be better to update TableGen with this logic instead of putting this in ISel? Also, we should probably include hasUnmodeledSideEffects along with mayLoad and mayStore. -Hal> > Tim.-- Hal Finkel Postdoctoral Appointee Leadership Computing Facility Argonne National Laboratory
On Sat, 23 Jun 2012 21:18:37 -0500 Hal Finkel <hfinkel at anl.gov> wrote:> On Sat, 23 Jun 2012 22:28:55 +0100 > Tim Northover <t.p.northover at gmail.com> wrote: > > > On Sat, Jun 23, 2012 at 04:10:51PM -0500, Hal Finkel wrote: > > > Working on a target I added this pattern: > > > > > > def : Pat<(v4i64 (load xoaddr:$src)), > > > (QVFCTIDb (QVLFDXb xoaddr:$src))>; > > > > > > I'd like to fix this so that it works correctly: the TokenFactor > > > inputs should be attached to all inner-most instructions. I'm > > > guessing this is somewhere in SelectionDAGISel.cpp, but if someone > > > has a more-specific idea, I'd appreciate hearing it. > > > > I believe it's a current TableGen limitation. When generating its > > DAG tables for this kind of thing, TableGen gives output > > instructions that should take chain a special flag: Opfl_Chain. > > > > Unfortunately the way it decides which instructions are worthy of > > this flag is rather naive: > > + If an instruction has a built-in pattern (in the Instruciton > > record), it checks whether that makes use of a chain. > > + Otherwise, the outer instruction of the Pat gets a chain. > > > > So if your QVLFDXb instruction doesn't set the Pattern(s?) field, > > TableGen won't know it needs the chain. > > Tim, > > Correct, the instruction has no pattern of its own. > > > > > There's a big comment just above the test about how the current > > situation is rather broken. I'm currently inclined to add a check > > for mayLoad and mayStore at that point in TableGeni (see patch), > > but was waiting until I could give tests and justification on the > > list before submitting it. > > Thanks for sending this!Please ignore this statement:> As far as a long-term solution, would it be > better to update TableGen with this logic instead of putting this in > ISel?-Hal> Also, we should probably include hasUnmodeledSideEffects along > with mayLoad and mayStore. > > -Hal > > > > > Tim. > > >-- Hal Finkel Postdoctoral Appointee Leadership Computing Facility Argonne National Laboratory
On Sat, 23 Jun 2012 21:25:48 -0500 Hal Finkel <hfinkel at anl.gov> wrote:> On Sat, 23 Jun 2012 21:18:37 -0500 > Hal Finkel <hfinkel at anl.gov> wrote: > > > On Sat, 23 Jun 2012 22:28:55 +0100 > > Tim Northover <t.p.northover at gmail.com> wrote: > > > > > On Sat, Jun 23, 2012 at 04:10:51PM -0500, Hal Finkel wrote: > > > > Working on a target I added this pattern: > > > > > > > > def : Pat<(v4i64 (load xoaddr:$src)), > > > > (QVFCTIDb (QVLFDXb xoaddr:$src))>; > > > > > > > > I'd like to fix this so that it works correctly: the TokenFactor > > > > inputs should be attached to all inner-most instructions. I'm > > > > guessing this is somewhere in SelectionDAGISel.cpp, but if > > > > someone has a more-specific idea, I'd appreciate hearing it. > > > > > > I believe it's a current TableGen limitation. When generating its > > > DAG tables for this kind of thing, TableGen gives output > > > instructions that should take chain a special flag: Opfl_Chain. > > > > > > Unfortunately the way it decides which instructions are worthy of > > > this flag is rather naive: > > > + If an instruction has a built-in pattern (in the Instruciton > > > record), it checks whether that makes use of a chain. > > > + Otherwise, the outer instruction of the Pat gets a chain. > > > > > > So if your QVLFDXb instruction doesn't set the Pattern(s?) field, > > > TableGen won't know it needs the chain. > > > > Tim, > > > > Correct, the instruction has no pattern of its own. > > > > > > > > There's a big comment just above the test about how the current > > > situation is rather broken. I'm currently inclined to add a check > > > for mayLoad and mayStore at that point in TableGeni (see patch), > > > but was waiting until I could give tests and justification on the > > > list before submitting it. > > > > Thanks for sending this! > > Please ignore this statement: > > As far as a long-term solution, would it be > > better to update TableGen with this logic instead of putting this in > > ISel? > > -Hal > > > Also, we should probably include hasUnmodeledSideEffects along > > with mayLoad and mayStore.I also had to include II.canFoldAsLoad to make this work for me. As is the case with other "simple" loads in the PowerPC backend, canFoldAsLoad is set but mayLoad is not (is this wrong)? Thanks again, Hal> > > > -Hal > > > > > > > > Tim. > > > > > > > > >-- Hal Finkel Postdoctoral Appointee Leadership Computing Facility Argonne National Laboratory