[PATCH] D32491: [globalisel][tablegen] Compute available feature bits correctly.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/Target/Target.td:538
+  /// Ignored by SelectionDAG, it always recomputes the predicate on every use.
+  bit RecomputePerFunction = 0;
 }
----------------
rovka wrote:
> dsanders wrote:
> > rovka wrote:
> > > dsanders wrote:
> > > > rovka wrote:
> > > > > This looks very easy to forget to set when adding a new predicate. Would it make sense to have 2 subclasses of Predicate (ModulePredicate and FunctionPredicate) and define all the predicates based on them? Naturally, it would be a pretty big mechanical change to update all the targets, so it should be a separate patch, but I think it would make things easier to maintain in the long run. What do you think?
> > > > If your predicate references MF you'll get a compile error since MF isn't declared in computeAvailableModuleFeatures. Things implemented via class members (e.g. ForCodeSize but that one is ok because of the cache) will silently do the wrong thing. Even without that, I think renaming to ModulePredicate/FunctionPredicate would be a good change to make since it's clearer.
> > > > 
> > > > If we're going to rename Predicate, we should probably take the opportunity to add ISel/CG/CodeGen or similar to the name too.
> > > > 
> > > > One thing that's a bit weird here is that predicates using ForCodeSize and similar would be ModulePredicate's because of the cache in getSubtargetImpl() even though they're logically per-function predicates.
> > > Hmm, actually, in light of your more detailed explanations, I'm not so sure Module vs Function is the right abstraction here. The matter of whether or not a predicate is safe to cache doesn't seem to correlate as well as I was hoping with whether or not it's a function predicate. Maybe we should have something like CacheablePredicate (or StablePredicate? or ConstantPredicate?), and use that everywhere except where it's not safe to cache? That would force people to either decide that something is safe to cache or fall back to the slow-but-correct path of recomputing it every time.
> > > 
> > > Also, +1 for finding a better name for these predicates in general, I hate having to explain to people that this isn't about flag registers and condition codes every time :)
> > SubtargetPredicate and FunctionPredicate might be a better fit. SubtargetPredicate would cover both module-level predicates and function-level predicates that are part of the subtarget key.
> > 
> > > One thing that's a bit weird here is that predicates using ForCodeSize and similar would be
> > > ModulePredicate's because of the cache in getSubtargetImpl() even though they're logically
> > > per-function predicates.
> > 
> > Following on from the same line of thought, I think I've fixed this weirdness. It turns out that all of these can be moved to subtarget accessors without breaking SelectionDAG. This solves the scalability issue you mention on the AArch64InstructionSelector constructor. I've updated this patch with this change.
> > 
> > For naming RecomputePerFunction: How about RequiresMachineFunction? It dodges the issue of caching function-level information in the subtarget and it's a bit more obvious that it's how you get access to MF.
> RequiresMachineFunction is not bad, but is the way we access the MF really the issue here? I'm not convinced that the distinction between Function and MachineFunction is that important. Suppose we had the same caching system for MachineFunction as we do for Function - those predicates would still need to be recomputed since, as you explained, they could change by the time we reach our pass.
> 
> For pretty much the same reason, I don't think SubtargetPredicate is explicit enough. Predicates that change are very rare, so people will be tempted to just use SubtargetPredicate everywhere without giving it a second thought. We need something that will draw attention to the fact that the predicate may be cached, or at least something that sounds strange enough that it would make people check the comments to see what the big deal is.
> 
> Anyway, sorry about bikeshedding this so much :)
> RequiresMachineFunction is not bad, but is the way we access the MF really
> the issue here? I'm not convinced that the distinction between Function and 
> MachineFunction is that important. Suppose we had the same caching system 
> for MachineFunction as we do for Function - those predicates would still need 
> to be recomputed since, as you explained, they could change by the time we 
> reach our pass.

I was thinking it could be about whether MF is declared or not but this doesn't make sense now that I've realized my mistake below.

> For pretty much the same reason, I don't think SubtargetPredicate is explicit
> enough. Predicates that change are very rare, so people will be tempted to
> just use SubtargetPredicate everywhere without giving it a second thought.
> We need something that will draw attention to the fact that the predicate may
> be cached, or at least something that sounds strange enough that it would
> make people check the comments to see what the big deal is.

I see your point. For some reason I was thinking "as long as a predicate is accessing values from the Subtarget it's not important whether it's a cached value from the function" but that's obviously not true if we don't call computeAvailableModuleFeatures() again after swapping out a Subtarget.

> Anyway, sorry about bikeshedding this so much :)

No worries. If we don't pick a good enough name then we'll be doomed to explain it every time it confuses someone so I'd prefer to get it right.


https://reviews.llvm.org/D32491