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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D32491#737781, @rovka wrote:

> So, we have a new GISelAccessor with a new InstructionSelector for each unique subtarget (module + function attributes, as hashed in getSubtargetImpl). Why do we need to have module features and function features in each InstructionSelector? Isn't there a different InstructionSelector for each kind of function, and don't we magically get the right one when processing each function? What am I missing? It seems to me that we could get away with a single set of availableFeatures per InstructionSelector.


getSubtargetImp() only has access to the Function* but NotWin64WithoutFP needs the MachineFunction* to evaluate `Subtarget->getFrameLowering()->hasFP(*MF)`. It's possible to go from MachineFunction* to Function* but not the other way. Also, the value of hasFP() changes from pass to pass as the stack layout becomes more concrete.

> In any case, I think you need a more complex test with several different functions with different attributes to make sure you're computing the correct features for each function and you don't have any stale info lingering between them (something like what I do with function attributes in arm-instruction-select.mir, although that's just for convenience and not for this exact purpose).

Ok.



================
Comment at: include/llvm/Target/Target.td:538
+  /// Ignored by SelectionDAG, it always recomputes the predicate on every use.
+  bit RecomputePerFunction = 0;
 }
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:53
+                             const AArch64RegisterBankInfo &RBI,
+                             bool ForCodeSize);
 
----------------
rovka wrote:
> This doesn't look like it would scale very well if we needed to add more function-level predicates. Is there any significant disadvantage to threading the MachineFunction all the way down here? Then we'd only have to update the constructor, without createXInstructionSelector etc.
The caller doesn't have access to MachineFunction so I assume you meant Function. I'm not sure about threading that down since it moves the predicate testing away from the cache key generator. It would be easy to add a predicate in the instruction selector and forget to update the key generator. I agree we need to do something here though. I'll have a think about it.


================
Comment at: lib/Target/AArch64/AArch64TargetMachine.cpp:266
                        : TargetFS;
+  std::string ForCodeSizeStr = ForCodeSize ? "-forcodesize" : "";
 
----------------
rovka wrote:
> Nit: I guess it doesn't matter much, since this is only used for hashing, but it would be nice to keep the convention used for the target features etc (",+forcodesize").
Ok. And presumably we'd have "-forcodesize" instead of "" if we're matching that convention.


https://reviews.llvm.org/D32491