llvm dev - Jun 2012 - [LLVMdev] Complex load patterns and token factors

Working on a target I added this pattern:

def : Pat<(v4i64 (load xoaddr:$src)),
          (QVFCTIDb (QVLFDXb xoaddr:$src))>;

which represents an actual load followed by a necessary conversion
operation. The problem is that when this matches any TokenFactor that
was attached to the load node gets attached, not to the inner load
instruction, but the outer conversion operation. This is causing
miscompiles because stores that should be scheduled before the load end
up scheduled after the load (but before the conversion operation,
because the conversion operation inherited the TokenFactor input).

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.

Thanks in advance,
Hal

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory

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.

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.

Tim.
-------------- next part --------------
diff --git a/utils/TableGen/DAGISelMatcherGen.cpp
b/utils/TableGen/DAGISelMatcherGen.cpp
index 2ac7b87..4acd209 100644
--- a/utils/TableGen/DAGISelMatcherGen.cpp
+++ b/utils/TableGen/DAGISelMatcherGen.cpp
@@ -690,6 +690,12 @@ EmitResultInstructionAsOperand(const TreePatternNode *N,
   bool NodeHasChain = InstPatNode &&
                       InstPatNode->TreeHasProperty(SDNPHasChain, CGP);
 
+  // Instructions which load and store from memory should have a chain,
+  // regardless of whether they happen to have an internal pattern saying so.
+  if (Pattern.getSrcPattern()->TreeHasProperty(SDNPHasChain, CGP)
+      && (II.mayLoad || II.mayStore))
+      NodeHasChain = true;
+
   bool isRoot = N == Pattern.getDstPattern();
 
   // TreeHasOutGlue - True if this tree has glue.

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