[PATCH] Small refactor on VectorizerHint for deduplication

[Arnold Schwaighofer]

> On Aug 19, 2014, at 7:48 AM, Renato Golin <renato.golin at linaro.org> wrote:
> 
> Hi Arnold,
> 
> Thanks for your review. Replies inline.
> 
> 
>>> ! In D4913#10, @aschwaighofer wrote:
>> I think this would reads easier if we refer to Hint and Hints instead of HintType(s).
> 
> Fair enough, I can change that.
> 
> 
>> We concatenate and split "llvm.loop" and "vectorize.foo". I think it would simplify the code if the Hints refer to the full string?
> 
> We do that to validate and only warn if it's an invalid "llvm.loop." metadata, so we don't even scan the others (like tbaa, etc). Adding it to the name will simplify the pattern match on the hint but would make warning a bit harder. I will move the matching on the prefix to the parseHint() to make it a bit more isolated, and easier to change in the future.

You can leave the one prefix check that makes sure we don’t look at anything but “llvm.loop” metadata 

    // Check if the hint starts with the loop metadata prefix.
      StringRef Hint = S->getString();
      if (!Hint.startswith(Prefix()))
        continue;

I think the rest of the code could use the full string. Can you point me to the code that needs the suffix for the warning?

 
> 
> 
>> Can we centralize the logic if we move the validation logic into a member function "validate" that switches on a hint kind? Validation and the hint are intrinsically connected.
> 
> So, I though about using an enum to validate, but that meant we'd have to construct a new Hint from a string and assign a kind just to check if it's a valid hint at all, and then add InvalidHint objects and making sure none of the objects are duplicated. It got a bit complex...
> 
> Since we don't use static initializers, and this is only built once, and the constructor is statically set on the right validation lambda for each string, I thought it'd be at least safe. I agree it's not pretty.
> 
> I'll give it a few more tries…


The struct could be:


struct Hint {
  enum {
    kUnroll,
    kWidth,
    kForce,
 } Kind;

 const char *Name;
 unsigned Value;

 Hint(Kind k, const char *n, unsigned d) :kind(k), name(name), Value(d) {}
 
bool validate(unsigned V) {
    switch (kind) {
      case kUnroll:
        … validation
      case kWidth: ...
    }
 }

};

Then you could still write:

for (auto &H : Hints)
  if(H.Name == Hint)
    if(H.validate(Value)
      H.Value = Value;
    else {
      DEBUG(llvm::dbgs() << “…”);

And you could create the three hints like the patch is currently doing:

LoopVectorizeHints(const Loop *L, bool DisableUnrolling) :
  Width(Hint::kWidth, “llvm.loop.vectorize.width", VectorizationFactor),




Thanks,
Arnold